Create a Base store class for Zarr Store. - #612

Closed
Carreau wants to merge 9 commits into
zarr-developers:masterfrom
Carreau:base-store
Closed

Create a Base store class for Zarr Store.#612
Carreau wants to merge 9 commits into
zarr-developers:masterfrom
Carreau:base-store

Conversation

@Carreau

Copy link
Copy Markdown
Contributor

In progress,

All existing stores in zarr-python now inherit from this; and thus all the test stop testing for a close() method and call it unconditionally.

The base store is a bit more strict in what it accepts than subclasses (only allow strings), as it is generally safer for the superclass to be stricter as anything that works with superclass will work with subclass, but the opposite is untrue.

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • AppVeyor and Travis CI passes
  • Test coverage is 100% (Coveralls passes)

@Carreau

Copy link
Copy Markdown
ContributorAuthor

May want to write a store "wrapper", that handle usual mutable mapping and offer the right methods.

@pep8speaks

pep8speaks commented Oct 22, 2020

Copy link
Copy Markdown

Hello @Carreau! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-03-10 21:00:31 UTC

@codecov

codecovBot commented Oct 26, 2020

Copy link
Copy Markdown

Codecov Report

Merging #612 (566d145) into master (17728e8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #612 +/- ##
========================================
Coverage 99.93% 99.94% ========================================
Files 26 28 +2 Lines 9945 10307 +362 ========================================
+ Hits 9939 10301 +362 
Misses 6 6 
Impacted FilesCoverage Δ
zarr/convenience.py100.00% <100.00%> (ø)
zarr/core.py100.00% <100.00%> (ø)
zarr/creation.py100.00% <100.00%> (ø)
zarr/hierarchy.py100.00% <100.00%> (ø)
zarr/storage.py100.00% <100.00%> (ø)
zarr/tests/test_convenience.py100.00% <100.00%> (ø)
zarr/tests/test_core.py100.00% <100.00%> (ø)
zarr/tests/test_creation.py100.00% <100.00%> (ø)
zarr/tests/test_hierarchy.py100.00% <100.00%> (ø)
zarr/tests/test_storage.py100.00% <100.00%> (ø)
... and 3 more

Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/hierarchy.py Outdated
Comment threadzarr/storage.py Outdated
# def keys()
# def items()
# def values()
# def get

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleanup this,

Comment threadzarr/hierarchy.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/tests/test_hierarchy.py Outdated
@Carreau
Carreauforce-pushed the base-store branch 2 times, most recently from 01a68d5 to 0437842CompareNovember 6, 2020 21:54
@Carreau
Carreauforce-pushed the base-store branch 2 times, most recently from 14392e9 to 19fe17cCompareNovember 18, 2020 17:45
@Carreau
Carreau marked this pull request as ready for review December 2, 2020 18:45
@CarreauCarreau added this to the v2.7 milestone Dec 2, 2020
Unconditionally close store in tests.
All the tested stores should now have a `close()` method we can call and
will be no-op if the stores do not need closing.
Turn UserWarnings into errors
And turn then back into only warnings into relevant tests.
This ensure that we are not using deprecated functionalities, except
when testing for it.
initially based on 318eddcd, and later 1249f35 and 0f89a96
@Carreau

Copy link
Copy Markdown
ContributorAuthor

@joshmoore test should be passing, only a rebase and some cleanup might still be necessary after the rebase to get coverage up to 100%. Do you want me to fork master into a dev branch and target that for a dev branch ?

@joshmoore

Copy link
Copy Markdown
Member

Do you want me to fork master into a dev branch and target that for a dev branch ?

@Carreau, I didn't receive any objections to considering the mainline a pre-release while you get these branches in. Only requirement from my side would be to get 2.7.0 out the door. If we need a quick 2.7.x before you're done merging, then that would need to be from a stable branch.

@joshmoore

Copy link
Copy Markdown
Member

Having a think last night, what would everyone say to having this be the base for a v3 branch? Perhaps even start releasing pre-releases ASAP.

@joshmoore

Copy link
Copy Markdown
Member

Closing in favor of #789

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@Carreau@pep8speaks@joshmoore
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Create a Base store class for Zarr Store. - #612

Closed
Carreau wants to merge 9 commits into
zarr-developers:masterfrom
Carreau:base-store
Closed

Create a Base store class for Zarr Store.#612
Carreau wants to merge 9 commits into
zarr-developers:masterfrom
Carreau:base-store

Conversation

@Carreau

Copy link
Copy Markdown
Contributor

In progress,

All existing stores in zarr-python now inherit from this; and thus all the test stop testing for a close() method and call it unconditionally.

The base store is a bit more strict in what it accepts than subclasses (only allow strings), as it is generally safer for the superclass to be stricter as anything that works with superclass will work with subclass, but the opposite is untrue.

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • AppVeyor and Travis CI passes
  • Test coverage is 100% (Coveralls passes)

@Carreau

Copy link
Copy Markdown
ContributorAuthor

May want to write a store "wrapper", that handle usual mutable mapping and offer the right methods.

@pep8speaks

pep8speaks commented Oct 22, 2020

Copy link
Copy Markdown

Hello @Carreau! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-03-10 21:00:31 UTC

@codecov

codecovBot commented Oct 26, 2020

Copy link
Copy Markdown

Codecov Report

Merging #612 (566d145) into master (17728e8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #612 +/- ##
========================================
Coverage 99.93% 99.94% ========================================
Files 26 28 +2 Lines 9945 10307 +362 ========================================
+ Hits 9939 10301 +362 
Misses 6 6 
Impacted FilesCoverage Δ
zarr/convenience.py100.00% <100.00%> (ø)
zarr/core.py100.00% <100.00%> (ø)
zarr/creation.py100.00% <100.00%> (ø)
zarr/hierarchy.py100.00% <100.00%> (ø)
zarr/storage.py100.00% <100.00%> (ø)
zarr/tests/test_convenience.py100.00% <100.00%> (ø)
zarr/tests/test_core.py100.00% <100.00%> (ø)
zarr/tests/test_creation.py100.00% <100.00%> (ø)
zarr/tests/test_hierarchy.py100.00% <100.00%> (ø)
zarr/tests/test_storage.py100.00% <100.00%> (ø)
... and 3 more

Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/hierarchy.py Outdated
Comment threadzarr/storage.py Outdated
# def keys()
# def items()
# def values()
# def get

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleanup this,

Comment threadzarr/hierarchy.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/tests/test_hierarchy.py Outdated
@Carreau
Carreauforce-pushed the base-store branch 2 times, most recently from 01a68d5 to 0437842CompareNovember 6, 2020 21:54
@Carreau
Carreauforce-pushed the base-store branch 2 times, most recently from 14392e9 to 19fe17cCompareNovember 18, 2020 17:45
@Carreau
Carreau marked this pull request as ready for review December 2, 2020 18:45
@CarreauCarreau added this to the v2.7 milestone Dec 2, 2020
Unconditionally close store in tests.
All the tested stores should now have a `close()` method we can call and
will be no-op if the stores do not need closing.
Turn UserWarnings into errors
And turn then back into only warnings into relevant tests.
This ensure that we are not using deprecated functionalities, except
when testing for it.
initially based on 318eddcd, and later 1249f35 and 0f89a96
@Carreau

Copy link
Copy Markdown
ContributorAuthor

@joshmoore test should be passing, only a rebase and some cleanup might still be necessary after the rebase to get coverage up to 100%. Do you want me to fork master into a dev branch and target that for a dev branch ?

@joshmoore

Copy link
Copy Markdown
Member

Do you want me to fork master into a dev branch and target that for a dev branch ?

@Carreau, I didn't receive any objections to considering the mainline a pre-release while you get these branches in. Only requirement from my side would be to get 2.7.0 out the door. If we need a quick 2.7.x before you're done merging, then that would need to be from a stable branch.

@joshmoore

Copy link
Copy Markdown
Member

Having a think last night, what would everyone say to having this be the base for a v3 branch? Perhaps even start releasing pre-releases ASAP.

@joshmoore

Copy link
Copy Markdown
Member

Closing in favor of #789

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@Carreau@pep8speaks@joshmoore
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Create a Base store class for Zarr Store. - #612

Closed
Carreau wants to merge 9 commits into
zarr-developers:masterfrom
Carreau:base-store
Closed

Create a Base store class for Zarr Store.#612
Carreau wants to merge 9 commits into
zarr-developers:masterfrom
Carreau:base-store

Conversation

@Carreau

Copy link
Copy Markdown
Contributor

In progress,

All existing stores in zarr-python now inherit from this; and thus all the test stop testing for a close() method and call it unconditionally.

The base store is a bit more strict in what it accepts than subclasses (only allow strings), as it is generally safer for the superclass to be stricter as anything that works with superclass will work with subclass, but the opposite is untrue.

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • AppVeyor and Travis CI passes
  • Test coverage is 100% (Coveralls passes)

@Carreau

Copy link
Copy Markdown
ContributorAuthor

May want to write a store "wrapper", that handle usual mutable mapping and offer the right methods.

@pep8speaks

pep8speaks commented Oct 22, 2020

Copy link
Copy Markdown

Hello @Carreau! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-03-10 21:00:31 UTC

@codecov

codecovBot commented Oct 26, 2020

Copy link
Copy Markdown

Codecov Report

Merging #612 (566d145) into master (17728e8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #612 +/- ##
========================================
Coverage 99.93% 99.94% ========================================
Files 26 28 +2 Lines 9945 10307 +362 ========================================
+ Hits 9939 10301 +362 
Misses 6 6 
Impacted FilesCoverage Δ
zarr/convenience.py100.00% <100.00%> (ø)
zarr/core.py100.00% <100.00%> (ø)
zarr/creation.py100.00% <100.00%> (ø)
zarr/hierarchy.py100.00% <100.00%> (ø)
zarr/storage.py100.00% <100.00%> (ø)
zarr/tests/test_convenience.py100.00% <100.00%> (ø)
zarr/tests/test_core.py100.00% <100.00%> (ø)
zarr/tests/test_creation.py100.00% <100.00%> (ø)
zarr/tests/test_hierarchy.py100.00% <100.00%> (ø)
zarr/tests/test_storage.py100.00% <100.00%> (ø)
... and 3 more

Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/hierarchy.py Outdated
Comment threadzarr/storage.py Outdated
# def keys()
# def items()
# def values()
# def get

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleanup this,

Comment threadzarr/hierarchy.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/tests/test_hierarchy.py Outdated
@Carreau
Carreauforce-pushed the base-store branch 2 times, most recently from 01a68d5 to 0437842CompareNovember 6, 2020 21:54
@Carreau
Carreauforce-pushed the base-store branch 2 times, most recently from 14392e9 to 19fe17cCompareNovember 18, 2020 17:45
@Carreau
Carreau marked this pull request as ready for review December 2, 2020 18:45
@CarreauCarreau added this to the v2.7 milestone Dec 2, 2020
Unconditionally close store in tests.
All the tested stores should now have a `close()` method we can call and
will be no-op if the stores do not need closing.
Turn UserWarnings into errors
And turn then back into only warnings into relevant tests.
This ensure that we are not using deprecated functionalities, except
when testing for it.
initially based on 318eddcd, and later 1249f35 and 0f89a96
@Carreau

Copy link
Copy Markdown
ContributorAuthor

@joshmoore test should be passing, only a rebase and some cleanup might still be necessary after the rebase to get coverage up to 100%. Do you want me to fork master into a dev branch and target that for a dev branch ?

@joshmoore

Copy link
Copy Markdown
Member

Do you want me to fork master into a dev branch and target that for a dev branch ?

@Carreau, I didn't receive any objections to considering the mainline a pre-release while you get these branches in. Only requirement from my side would be to get 2.7.0 out the door. If we need a quick 2.7.x before you're done merging, then that would need to be from a stable branch.

@joshmoore

Copy link
Copy Markdown
Member

Having a think last night, what would everyone say to having this be the base for a v3 branch? Perhaps even start releasing pre-releases ASAP.

@joshmoore

Copy link
Copy Markdown
Member

Closing in favor of #789

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@Carreau@pep8speaks@joshmoore
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Create a Base store class for Zarr Store. - #612

Closed
Carreau wants to merge 9 commits into
zarr-developers:masterfrom
Carreau:base-store
Closed

Create a Base store class for Zarr Store.#612
Carreau wants to merge 9 commits into
zarr-developers:masterfrom
Carreau:base-store

Conversation

@Carreau

Copy link
Copy Markdown
Contributor

In progress,

All existing stores in zarr-python now inherit from this; and thus all the test stop testing for a close() method and call it unconditionally.

The base store is a bit more strict in what it accepts than subclasses (only allow strings), as it is generally safer for the superclass to be stricter as anything that works with superclass will work with subclass, but the opposite is untrue.

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • AppVeyor and Travis CI passes
  • Test coverage is 100% (Coveralls passes)

@Carreau

Copy link
Copy Markdown
ContributorAuthor

May want to write a store "wrapper", that handle usual mutable mapping and offer the right methods.

@pep8speaks

pep8speaks commented Oct 22, 2020

Copy link
Copy Markdown

Hello @Carreau! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-03-10 21:00:31 UTC

@codecov

codecovBot commented Oct 26, 2020

Copy link
Copy Markdown

Codecov Report

Merging #612 (566d145) into master (17728e8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #612 +/- ##
========================================
Coverage 99.93% 99.94% ========================================
Files 26 28 +2 Lines 9945 10307 +362 ========================================
+ Hits 9939 10301 +362 
Misses 6 6 
Impacted FilesCoverage Δ
zarr/convenience.py100.00% <100.00%> (ø)
zarr/core.py100.00% <100.00%> (ø)
zarr/creation.py100.00% <100.00%> (ø)
zarr/hierarchy.py100.00% <100.00%> (ø)
zarr/storage.py100.00% <100.00%> (ø)
zarr/tests/test_convenience.py100.00% <100.00%> (ø)
zarr/tests/test_core.py100.00% <100.00%> (ø)
zarr/tests/test_creation.py100.00% <100.00%> (ø)
zarr/tests/test_hierarchy.py100.00% <100.00%> (ø)
zarr/tests/test_storage.py100.00% <100.00%> (ø)
... and 3 more

Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/hierarchy.py Outdated
Comment threadzarr/storage.py Outdated
# def keys()
# def items()
# def values()
# def get

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleanup this,

Comment threadzarr/hierarchy.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/tests/test_hierarchy.py Outdated
@Carreau
Carreauforce-pushed the base-store branch 2 times, most recently from 01a68d5 to 0437842CompareNovember 6, 2020 21:54
@Carreau
Carreauforce-pushed the base-store branch 2 times, most recently from 14392e9 to 19fe17cCompareNovember 18, 2020 17:45
@Carreau
Carreau marked this pull request as ready for review December 2, 2020 18:45
@CarreauCarreau added this to the v2.7 milestone Dec 2, 2020
Unconditionally close store in tests.
All the tested stores should now have a `close()` method we can call and
will be no-op if the stores do not need closing.
Turn UserWarnings into errors
And turn then back into only warnings into relevant tests.
This ensure that we are not using deprecated functionalities, except
when testing for it.
initially based on 318eddcd, and later 1249f35 and 0f89a96
@Carreau

Copy link
Copy Markdown
ContributorAuthor

@joshmoore test should be passing, only a rebase and some cleanup might still be necessary after the rebase to get coverage up to 100%. Do you want me to fork master into a dev branch and target that for a dev branch ?

@joshmoore

Copy link
Copy Markdown
Member

Do you want me to fork master into a dev branch and target that for a dev branch ?

@Carreau, I didn't receive any objections to considering the mainline a pre-release while you get these branches in. Only requirement from my side would be to get 2.7.0 out the door. If we need a quick 2.7.x before you're done merging, then that would need to be from a stable branch.

@joshmoore

Copy link
Copy Markdown
Member

Having a think last night, what would everyone say to having this be the base for a v3 branch? Perhaps even start releasing pre-releases ASAP.

@joshmoore

Copy link
Copy Markdown
Member

Closing in favor of #789

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@Carreau@pep8speaks@joshmoore
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Create a Base store class for Zarr Store. - #612

Closed
Carreau wants to merge 9 commits into
zarr-developers:masterfrom
Carreau:base-store
Closed

Create a Base store class for Zarr Store.#612
Carreau wants to merge 9 commits into
zarr-developers:masterfrom
Carreau:base-store

Conversation

@Carreau

Copy link
Copy Markdown
Contributor

In progress,

All existing stores in zarr-python now inherit from this; and thus all the test stop testing for a close() method and call it unconditionally.

The base store is a bit more strict in what it accepts than subclasses (only allow strings), as it is generally safer for the superclass to be stricter as anything that works with superclass will work with subclass, but the opposite is untrue.

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • AppVeyor and Travis CI passes
  • Test coverage is 100% (Coveralls passes)

@Carreau

Copy link
Copy Markdown
ContributorAuthor

May want to write a store "wrapper", that handle usual mutable mapping and offer the right methods.

@pep8speaks

pep8speaks commented Oct 22, 2020

Copy link
Copy Markdown

Hello @Carreau! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-03-10 21:00:31 UTC

@codecov

codecovBot commented Oct 26, 2020

Copy link
Copy Markdown

Codecov Report

Merging #612 (566d145) into master (17728e8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #612 +/- ##
========================================
Coverage 99.93% 99.94% ========================================
Files 26 28 +2 Lines 9945 10307 +362 ========================================
+ Hits 9939 10301 +362 
Misses 6 6 
Impacted FilesCoverage Δ
zarr/convenience.py100.00% <100.00%> (ø)
zarr/core.py100.00% <100.00%> (ø)
zarr/creation.py100.00% <100.00%> (ø)
zarr/hierarchy.py100.00% <100.00%> (ø)
zarr/storage.py100.00% <100.00%> (ø)
zarr/tests/test_convenience.py100.00% <100.00%> (ø)
zarr/tests/test_core.py100.00% <100.00%> (ø)
zarr/tests/test_creation.py100.00% <100.00%> (ø)
zarr/tests/test_hierarchy.py100.00% <100.00%> (ø)
zarr/tests/test_storage.py100.00% <100.00%> (ø)
... and 3 more

Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/hierarchy.py Outdated
Comment threadzarr/storage.py Outdated
# def keys()
# def items()
# def values()
# def get

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleanup this,

Comment threadzarr/hierarchy.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/tests/test_hierarchy.py Outdated
@Carreau
Carreauforce-pushed the base-store branch 2 times, most recently from 01a68d5 to 0437842CompareNovember 6, 2020 21:54
@Carreau
Carreauforce-pushed the base-store branch 2 times, most recently from 14392e9 to 19fe17cCompareNovember 18, 2020 17:45
@Carreau
Carreau marked this pull request as ready for review December 2, 2020 18:45
@CarreauCarreau added this to the v2.7 milestone Dec 2, 2020
Unconditionally close store in tests.
All the tested stores should now have a `close()` method we can call and
will be no-op if the stores do not need closing.
Turn UserWarnings into errors
And turn then back into only warnings into relevant tests.
This ensure that we are not using deprecated functionalities, except
when testing for it.
initially based on 318eddcd, and later 1249f35 and 0f89a96
@Carreau

Copy link
Copy Markdown
ContributorAuthor

@joshmoore test should be passing, only a rebase and some cleanup might still be necessary after the rebase to get coverage up to 100%. Do you want me to fork master into a dev branch and target that for a dev branch ?

@joshmoore

Copy link
Copy Markdown
Member

Do you want me to fork master into a dev branch and target that for a dev branch ?

@Carreau, I didn't receive any objections to considering the mainline a pre-release while you get these branches in. Only requirement from my side would be to get 2.7.0 out the door. If we need a quick 2.7.x before you're done merging, then that would need to be from a stable branch.

@joshmoore

Copy link
Copy Markdown
Member

Having a think last night, what would everyone say to having this be the base for a v3 branch? Perhaps even start releasing pre-releases ASAP.

@joshmoore

Copy link
Copy Markdown
Member

Closing in favor of #789

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@Carreau@pep8speaks@joshmoore
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Create a Base store class for Zarr Store. - #612

Closed
Carreau wants to merge 9 commits into
zarr-developers:masterfrom
Carreau:base-store
Closed

Create a Base store class for Zarr Store.#612
Carreau wants to merge 9 commits into
zarr-developers:masterfrom
Carreau:base-store

Conversation

@Carreau

Copy link
Copy Markdown
Contributor

In progress,

All existing stores in zarr-python now inherit from this; and thus all the test stop testing for a close() method and call it unconditionally.

The base store is a bit more strict in what it accepts than subclasses (only allow strings), as it is generally safer for the superclass to be stricter as anything that works with superclass will work with subclass, but the opposite is untrue.

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • AppVeyor and Travis CI passes
  • Test coverage is 100% (Coveralls passes)

@Carreau

Copy link
Copy Markdown
ContributorAuthor

May want to write a store "wrapper", that handle usual mutable mapping and offer the right methods.

@pep8speaks

pep8speaks commented Oct 22, 2020

Copy link
Copy Markdown

Hello @Carreau! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-03-10 21:00:31 UTC

@codecov

codecovBot commented Oct 26, 2020

Copy link
Copy Markdown

Codecov Report

Merging #612 (566d145) into master (17728e8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #612 +/- ##
========================================
Coverage 99.93% 99.94% ========================================
Files 26 28 +2 Lines 9945 10307 +362 ========================================
+ Hits 9939 10301 +362 
Misses 6 6 
Impacted FilesCoverage Δ
zarr/convenience.py100.00% <100.00%> (ø)
zarr/core.py100.00% <100.00%> (ø)
zarr/creation.py100.00% <100.00%> (ø)
zarr/hierarchy.py100.00% <100.00%> (ø)
zarr/storage.py100.00% <100.00%> (ø)
zarr/tests/test_convenience.py100.00% <100.00%> (ø)
zarr/tests/test_core.py100.00% <100.00%> (ø)
zarr/tests/test_creation.py100.00% <100.00%> (ø)
zarr/tests/test_hierarchy.py100.00% <100.00%> (ø)
zarr/tests/test_storage.py100.00% <100.00%> (ø)
... and 3 more

Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/hierarchy.py Outdated
Comment threadzarr/storage.py Outdated
# def keys()
# def items()
# def values()
# def get

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleanup this,

Comment threadzarr/hierarchy.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/tests/test_hierarchy.py Outdated
@Carreau
Carreauforce-pushed the base-store branch 2 times, most recently from 01a68d5 to 0437842CompareNovember 6, 2020 21:54
@Carreau
Carreauforce-pushed the base-store branch 2 times, most recently from 14392e9 to 19fe17cCompareNovember 18, 2020 17:45
@Carreau
Carreau marked this pull request as ready for review December 2, 2020 18:45
@CarreauCarreau added this to the v2.7 milestone Dec 2, 2020
Unconditionally close store in tests.
All the tested stores should now have a `close()` method we can call and
will be no-op if the stores do not need closing.
Turn UserWarnings into errors
And turn then back into only warnings into relevant tests.
This ensure that we are not using deprecated functionalities, except
when testing for it.
initially based on 318eddcd, and later 1249f35 and 0f89a96
@Carreau

Copy link
Copy Markdown
ContributorAuthor

@joshmoore test should be passing, only a rebase and some cleanup might still be necessary after the rebase to get coverage up to 100%. Do you want me to fork master into a dev branch and target that for a dev branch ?

@joshmoore

Copy link
Copy Markdown
Member

Do you want me to fork master into a dev branch and target that for a dev branch ?

@Carreau, I didn't receive any objections to considering the mainline a pre-release while you get these branches in. Only requirement from my side would be to get 2.7.0 out the door. If we need a quick 2.7.x before you're done merging, then that would need to be from a stable branch.

@joshmoore

Copy link
Copy Markdown
Member

Having a think last night, what would everyone say to having this be the base for a v3 branch? Perhaps even start releasing pre-releases ASAP.

@joshmoore

Copy link
Copy Markdown
Member

Closing in favor of #789

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@Carreau@pep8speaks@joshmoore
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Create a Base store class for Zarr Store. - #612

Closed
Carreau wants to merge 9 commits into
zarr-developers:masterfrom
Carreau:base-store
Closed

Create a Base store class for Zarr Store.#612
Carreau wants to merge 9 commits into
zarr-developers:masterfrom
Carreau:base-store

Conversation

@Carreau

Copy link
Copy Markdown
Contributor

In progress,

All existing stores in zarr-python now inherit from this; and thus all the test stop testing for a close() method and call it unconditionally.

The base store is a bit more strict in what it accepts than subclasses (only allow strings), as it is generally safer for the superclass to be stricter as anything that works with superclass will work with subclass, but the opposite is untrue.

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • AppVeyor and Travis CI passes
  • Test coverage is 100% (Coveralls passes)

@Carreau

Copy link
Copy Markdown
ContributorAuthor

May want to write a store "wrapper", that handle usual mutable mapping and offer the right methods.

@pep8speaks

pep8speaks commented Oct 22, 2020

Copy link
Copy Markdown

Hello @Carreau! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-03-10 21:00:31 UTC

@codecov

codecovBot commented Oct 26, 2020

Copy link
Copy Markdown

Codecov Report

Merging #612 (566d145) into master (17728e8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #612 +/- ##
========================================
Coverage 99.93% 99.94% ========================================
Files 26 28 +2 Lines 9945 10307 +362 ========================================
+ Hits 9939 10301 +362 
Misses 6 6 
Impacted FilesCoverage Δ
zarr/convenience.py100.00% <100.00%> (ø)
zarr/core.py100.00% <100.00%> (ø)
zarr/creation.py100.00% <100.00%> (ø)
zarr/hierarchy.py100.00% <100.00%> (ø)
zarr/storage.py100.00% <100.00%> (ø)
zarr/tests/test_convenience.py100.00% <100.00%> (ø)
zarr/tests/test_core.py100.00% <100.00%> (ø)
zarr/tests/test_creation.py100.00% <100.00%> (ø)
zarr/tests/test_hierarchy.py100.00% <100.00%> (ø)
zarr/tests/test_storage.py100.00% <100.00%> (ø)
... and 3 more

Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/hierarchy.py Outdated
Comment threadzarr/storage.py Outdated
# def keys()
# def items()
# def values()
# def get

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleanup this,

Comment threadzarr/hierarchy.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/tests/test_hierarchy.py Outdated
@Carreau
Carreauforce-pushed the base-store branch 2 times, most recently from 01a68d5 to 0437842CompareNovember 6, 2020 21:54
@Carreau
Carreauforce-pushed the base-store branch 2 times, most recently from 14392e9 to 19fe17cCompareNovember 18, 2020 17:45
@Carreau
Carreau marked this pull request as ready for review December 2, 2020 18:45
@CarreauCarreau added this to the v2.7 milestone Dec 2, 2020
Unconditionally close store in tests.
All the tested stores should now have a `close()` method we can call and
will be no-op if the stores do not need closing.
Turn UserWarnings into errors
And turn then back into only warnings into relevant tests.
This ensure that we are not using deprecated functionalities, except
when testing for it.
initially based on 318eddcd, and later 1249f35 and 0f89a96
@Carreau

Copy link
Copy Markdown
ContributorAuthor

@joshmoore test should be passing, only a rebase and some cleanup might still be necessary after the rebase to get coverage up to 100%. Do you want me to fork master into a dev branch and target that for a dev branch ?

@joshmoore

Copy link
Copy Markdown
Member

Do you want me to fork master into a dev branch and target that for a dev branch ?

@Carreau, I didn't receive any objections to considering the mainline a pre-release while you get these branches in. Only requirement from my side would be to get 2.7.0 out the door. If we need a quick 2.7.x before you're done merging, then that would need to be from a stable branch.

@joshmoore

Copy link
Copy Markdown
Member

Having a think last night, what would everyone say to having this be the base for a v3 branch? Perhaps even start releasing pre-releases ASAP.

@joshmoore

Copy link
Copy Markdown
Member

Closing in favor of #789

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@Carreau@pep8speaks@joshmoore
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Create a Base store class for Zarr Store. - #612

Closed
Carreau wants to merge 9 commits into
zarr-developers:masterfrom
Carreau:base-store
Closed

Create a Base store class for Zarr Store.#612
Carreau wants to merge 9 commits into
zarr-developers:masterfrom
Carreau:base-store

Conversation

@Carreau

Copy link
Copy Markdown
Contributor

In progress,

All existing stores in zarr-python now inherit from this; and thus all the test stop testing for a close() method and call it unconditionally.

The base store is a bit more strict in what it accepts than subclasses (only allow strings), as it is generally safer for the superclass to be stricter as anything that works with superclass will work with subclass, but the opposite is untrue.

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • AppVeyor and Travis CI passes
  • Test coverage is 100% (Coveralls passes)

@Carreau

Copy link
Copy Markdown
ContributorAuthor

May want to write a store "wrapper", that handle usual mutable mapping and offer the right methods.

@pep8speaks

pep8speaks commented Oct 22, 2020

Copy link
Copy Markdown

Hello @Carreau! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-03-10 21:00:31 UTC

@codecov

codecovBot commented Oct 26, 2020

Copy link
Copy Markdown

Codecov Report

Merging #612 (566d145) into master (17728e8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #612 +/- ##
========================================
Coverage 99.93% 99.94% ========================================
Files 26 28 +2 Lines 9945 10307 +362 ========================================
+ Hits 9939 10301 +362 
Misses 6 6 
Impacted FilesCoverage Δ
zarr/convenience.py100.00% <100.00%> (ø)
zarr/core.py100.00% <100.00%> (ø)
zarr/creation.py100.00% <100.00%> (ø)
zarr/hierarchy.py100.00% <100.00%> (ø)
zarr/storage.py100.00% <100.00%> (ø)
zarr/tests/test_convenience.py100.00% <100.00%> (ø)
zarr/tests/test_core.py100.00% <100.00%> (ø)
zarr/tests/test_creation.py100.00% <100.00%> (ø)
zarr/tests/test_hierarchy.py100.00% <100.00%> (ø)
zarr/tests/test_storage.py100.00% <100.00%> (ø)
... and 3 more

Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/hierarchy.py Outdated
Comment threadzarr/storage.py Outdated
# def keys()
# def items()
# def values()
# def get

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleanup this,

Comment threadzarr/hierarchy.py Outdated
Comment threadzarr/storage.py Outdated
Comment threadzarr/tests/test_hierarchy.py Outdated
@Carreau
Carreauforce-pushed the base-store branch 2 times, most recently from 01a68d5 to 0437842CompareNovember 6, 2020 21:54
@Carreau
Carreauforce-pushed the base-store branch 2 times, most recently from 14392e9 to 19fe17cCompareNovember 18, 2020 17:45
@Carreau
Carreau marked this pull request as ready for review December 2, 2020 18:45
@CarreauCarreau added this to the v2.7 milestone Dec 2, 2020
Unconditionally close store in tests.
All the tested stores should now have a `close()` method we can call and
will be no-op if the stores do not need closing.
Turn UserWarnings into errors
And turn then back into only warnings into relevant tests.
This ensure that we are not using deprecated functionalities, except
when testing for it.
initially based on 318eddcd, and later 1249f35 and 0f89a96
@Carreau

Copy link
Copy Markdown
ContributorAuthor

@joshmoore test should be passing, only a rebase and some cleanup might still be necessary after the rebase to get coverage up to 100%. Do you want me to fork master into a dev branch and target that for a dev branch ?

@joshmoore

Copy link
Copy Markdown
Member

Do you want me to fork master into a dev branch and target that for a dev branch ?

@Carreau, I didn't receive any objections to considering the mainline a pre-release while you get these branches in. Only requirement from my side would be to get 2.7.0 out the door. If we need a quick 2.7.x before you're done merging, then that would need to be from a stable branch.

@joshmoore

Copy link
Copy Markdown
Member

Having a think last night, what would everyone say to having this be the base for a v3 branch? Perhaps even start releasing pre-releases ASAP.

@joshmoore

Copy link
Copy Markdown
Member

Closing in favor of #789

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@Carreau@pep8speaks@joshmoore