zipstore delitems via override

[I-am-Emmanuel]

Fixes https://github.com/zarr-developers/zarr-python/issues/828

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
• [ ] GitHub Actions have all passed
• [ ] Test coverage is 100% (Codecov passes)

[I-am-Emmanuel]

I want to know if this is actually expected of me to do. Please if it is not give me a details explanation of what to do.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 96d11834ff3e2f618614d937ede7c07366b6b210 into eb6d14364280fa64b778913bc032aee783e1f65f - view on LGTM.com

new alerts:

• 1 for Unused local variable

[I-am-Emmanuel]

That's incredible. I'm not sure whether that is what is expected of me. I'll continue with the testing. Thanks

[jakirkham]

I think it should be doable and believe you are up to the task. I hope I've given enough info. As always though, please feel free to ask questions. Any question that occurs to you is worth asking 🙂

[I-am-Emmanuel]

Oh, just seeing this. I'm on it.

On Fri, Oct 14, 2022 at 1:55 AM jakirkham @.***> wrote:

I think it should be doable and believe you are up to the task. I hope I've given enough info. As always though, please feel free to ask questions. Any question that occurs to you is worth asking 🙂

— Reply to this email directly, view it on GitHub https://github.com/zarr-developers/zarr-python/pull/1184#issuecomment-1278338428, or unsubscribe https://github.com/notifications/unsubscribe-auth/AZBHSCVU5GTYMR62UFVAZBTWDCVQLANCNFSM6AAAAAAREQIMUY . You are receiving this because you authored the thread.Message ID: @.***>

[lgtm-com[bot]]

This pull request introduces 1 alert when merging a83bff2b06ceda1449939bce5048d0f7caaec38e into 2c52b172ead27a1d5918563096703d9f3ed0f5f5 - view on LGTM.com

new alerts:

• 1 for Unused local variable

[I-am-Emmanuel]

Something is perplexing me as I prepare to participate to the test. I see the test is checking for the pop item rather than the del item, therefore I rewrite a test for the previously worked-on function del_item. To minimize confusion, I also included a function for pop_items in the storeitem class.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 0144f953e99ec73f4d03a7e6825ea48578e9e600 into 2c52b172ead27a1d5918563096703d9f3ed0f5f5 - view on LGTM.com

new alerts:

• 1 for Unnecessary delete statement in function

[jakirkham]

Thanks Emmanuel! 🙏

Looks like many things are done. So marked them resolved above.

The linter raised a few stylistic issues. Went ahead and just added these to the PR for simplicity.

There is one more test remaining that I've commented on above.

There is a test failure we need to look at. Will give it some thought.

[jakirkham]

Think we need to tweak a few of ZipStore's methods to treat keys with values equal to b"" as missing. Once that is done this should fix the remaining test issues (and remove the need for special casing ZipStore in the tests)

[I-am-Emmanuel]

Waiting for this work to be reviewed.

[jakirkham]

Thanks @I-am-Emmanuel! 🙏

That's a good start. Think we still need similar changes to __contains__ and keylist (skipping empty entries)

[sanketverma1704]

@I-am-Emmanuel, would you like to finish this?