Skip to content

Conversation

@Carreau
Copy link
Contributor

@Carreau Carreau commented Dec 21, 2020

Again to get back to our 100% coverage.

@pep8speaks
Copy link

pep8speaks commented Dec 21, 2020

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 2020-12-22 02:05:57 UTC

@jhamman
Copy link
Member

jhamman commented Dec 21, 2020

@Carreau - per your request, here's a smallish-public Zarr store on Azure:

store = zarr.storage.ABSStore('carbonplan-share', prefix='zarr-demo', account_name='carbonplan')
zarr.open_consolidated(store)

@Carreau
Copy link
Contributor Author

Carreau commented Dec 21, 2020

Thanks super useful, it seem that querying '' on this instance works, but not on azurite.
So likely an azurite problem, I'll try to get an older impl of the package.

@jhamman
Copy link
Member

jhamman commented Dec 21, 2020

I wonder if #620 would fix the problem you are seeing with the empty string query.

@Carreau
Copy link
Contributor Author

Carreau commented Dec 22, 2020

I wonder if #620 would fix the problem you are seeing with the empty string query.

Possibly. I can have a look once we are back on track with testing.

This should increase coverage.
This is due to azurite not allowing blob keys to be the empty string
while azure seem to allow it.
@codecov
Copy link

codecov bot commented Dec 22, 2020

Codecov Report

Merging #680 (c389208) into master (6352b76) will increase coverage by 1.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #680      +/-   ##
==========================================
+ Coverage   98.38%   99.50%   +1.11%     
==========================================
  Files          29       29              
  Lines       10118    10127       +9     
==========================================
+ Hits         9955    10077     +122     
+ Misses        163       50     -113     
Impacted Files Coverage Δ
zarr/tests/test_core.py 100.00% <100.00%> (+0.77%) ⬆️
zarr/tests/test_storage.py 99.35% <100.00%> (+1.86%) ⬆️
zarr/tests/test_hierarchy.py 100.00% <0.00%> (+0.75%) ⬆️
zarr/storage.py 98.76% <0.00%> (+5.74%) ⬆️

@Carreau Carreau marked this pull request as ready for review January 1, 2021 19:50
@Carreau
Copy link
Contributor Author

Carreau commented Jan 1, 2021

This increases coverage; and I've open 681 to track reverting the xfail commits.

@Carreau Carreau merged commit aa786cb into zarr-developers:master Jan 1, 2021
@Carreau Carreau deleted the azr branch January 1, 2021 19:52
Sign up for free to 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