zipstore delitems via override

[I-am-Emmanuel]

Fixes #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]

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

new alerts:

• 1 for Unused local variable

[jakirkham]

Thanks Emmanuel! 🙏

Generally this seems like the right approach. Thanks for diving in 😄

Had a couple comments below on improvements we might want to make.

The next thing would be to start updating some tests that currently are not running (like this one), but should now be able to with this change.

Please let us know if you have any questions 🙂

[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 <#1184 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AZBHSCVU5GTYMR62UFVAZBTWDCVQLANCNFSM6AAAAAAREQIMUY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[lgtm-com]

This pull request introduces 1 alert when merging a83bff2 into 2c52b17 - 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]

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

new alerts:

• 1 for Unnecessary delete statement in function

[jakirkham]

Proposing a few changes below to fix lints

[jakirkham]

Since value is only used once, suggesting we inline it.

[jakirkham]

Would also update this test

[I-am-Emmanuel]

Okay.

[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?

[jakirkham]

Sorry had some review comments that didn't post. So completing to get those out

[jakirkham]

Suggested change

	raise KeyError("Key not found")
	raise KeyError(key)

[jakirkham]

Would suggest passing namelist through a set since there may be duplicated keys (thanks to overwrites).

Also with getinfo, this provides metadata about the file. Think we can just check the file size is non-zero (so no need to read the file).

Otherwise this seems reasonable.

Suggested change

	namelist = [key for key in self.zf.namelist() if self.zf.getinfo(key) != b""]
	namelist = [key for key in set(self.zf.namelist()) if self.zf.getinfo(key).file_size]

[jakirkham]

Suggested change

	raise KeyError
	raise KeyError(key)

[jhamman]

I'm going to close this as stale. Folks should feel free to reopen if there is interest in continuing this work. Note there is a new implementation of the Zip store in v3 that could use the same patch for whoever is interested.