bpo-37772: fix zipfile.Path.iterdir() outputs #15170
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ericvsmith
previously requested changes
Aug 9, 2019
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
Should both the tests be created in the same branch where I have this fix? Any thoughts or recommendations on how to have the old and new versions of _add_implied_dirs? Can I mock the old version in my test file? |
|
I mean just have one test. That test would fail when run against the code in master, but would succeed when run against your branch. So the net result is just a test checked in to your branch, and the test would succeed. But by knowing it fails when run against master, you know it's actually testing that your fix worked. I hope that makes sense. Edit: typo and grammar |
|
@shireenrao: The way I accomplish this test-driven-development approach with git is to create a different branch from master (or wherever your fix branched from master), write the test there. It should fail because you don't have your fix yet. Then, merge the test into the branch with the fix. Then, the tests should pass. Push this to the pull request and you should be good to go. Let me know if this feels too overwhelming and I'll be happy to take over. |
shireenrao
requested review from
1st1,
abalkin,
asvetlov,
pganssle,
rhettinger and
a team
as code owners
|
@ericvsmith - I added tests for the two scenarios explained in the original issue at https://bugs.python.org/issue37772. For my tests to work, I also had to add changes to add_dirs method in test_zipfile as this method had the same bug I fixed in zipfile.py. The tests succeed in my bugfix branch and fails in the master branch. I am concerned about the exception which gets thrown in the middle of a generator when running the assert statement. How do I gracefully fail this test? |
|
My Apologies Everybody! I think I messed up. I merged the latest changes from master into my bugfix branch and pushed it along with my new tests. |
|
I don’t think you messed up. If the diff looks right then you’re fine, and everything will get squashed on merge... so don’t worry about the noise from the merged commits. Alternately, you can always rebase your branch against master and force push. That will remove those commits. |
…example alpharep.
… of parent directories for each name.
jaraco
approved these changes
Aug 24, 2019
|
Thanks @shireenrao for the PR, and @jaraco for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8. |
|
I'm having trouble backporting to |
|
Thanks @shireenrao for the PR, and @jaraco for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8. |
|
GH-15461 is a backport of this pull request to the 3.8 branch. |
miss-islington
pushed a commit
to miss-islington/cpython
that referenced
this pull request
Aug 24, 2019
* fix Path._add_implied_dirs to include all implied directories * fix Path._add_implied_dirs to include all implied directories * Optimize code by using sets instead of lists * 📜🤖 Added by blurb_it. * fix Path._add_implied_dirs to include all implied directories * Optimize code by using sets instead of lists * 📜🤖 Added by blurb_it. * Add tests to zipfile.Path.iterdir() fix * Update test for zipfile.Path.iterdir() * remove whitespace from test file * Rewrite NEWS blurb to describe the user-facing impact and avoid implementation details. * remove redundant [] within set comprehension * Update to use unique_everseen to maintain order and other suggestions in review * remove whitespace and add back add_dirs in tests * Add new standalone function parents using posixpath to get parents of a directory * removing whitespace (sorry) * Remove import pathlib from zipfile.py * Rewrite _parents as a slice on a generator of the ancestry of a path. * Remove check for '.' and '/', now that parents no longer returns those. * Separate calculation of implied dirs from adding those * Re-use _implied_dirs in tests for generating zipfile with dir entries. * Replace three fixtures (abcde, abcdef, abde) with one representative example alpharep. * Simplify implementation of _implied_dirs by collapsing the generation of parent directories for each name. (cherry picked from commit a4e2991) Co-authored-by: shireenrao <shireenrao@gmail.com>
|
@jaraco - Thank you for your help. |
jaraco
pushed a commit
that referenced
this pull request
Aug 24, 2019
* fix Path._add_implied_dirs to include all implied directories * fix Path._add_implied_dirs to include all implied directories * Optimize code by using sets instead of lists * 📜🤖 Added by blurb_it. * fix Path._add_implied_dirs to include all implied directories * Optimize code by using sets instead of lists * 📜🤖 Added by blurb_it. * Add tests to zipfile.Path.iterdir() fix * Update test for zipfile.Path.iterdir() * remove whitespace from test file * Rewrite NEWS blurb to describe the user-facing impact and avoid implementation details. * remove redundant [] within set comprehension * Update to use unique_everseen to maintain order and other suggestions in review * remove whitespace and add back add_dirs in tests * Add new standalone function parents using posixpath to get parents of a directory * removing whitespace (sorry) * Remove import pathlib from zipfile.py * Rewrite _parents as a slice on a generator of the ancestry of a path. * Remove check for '.' and '/', now that parents no longer returns those. * Separate calculation of implied dirs from adding those * Re-use _implied_dirs in tests for generating zipfile with dir entries. * Replace three fixtures (abcde, abcdef, abde) with one representative example alpharep. * Simplify implementation of _implied_dirs by collapsing the generation of parent directories for each name. (cherry picked from commit a4e2991) Co-authored-by: shireenrao <shireenrao@gmail.com>
|
|
That looks like a false positive to me; ignoring. |
lisroach
pushed a commit
to lisroach/cpython
that referenced
this pull request
Sep 10, 2019
* fix Path._add_implied_dirs to include all implied directories * fix Path._add_implied_dirs to include all implied directories * Optimize code by using sets instead of lists * 📜🤖 Added by blurb_it. * fix Path._add_implied_dirs to include all implied directories * Optimize code by using sets instead of lists * 📜🤖 Added by blurb_it. * Add tests to zipfile.Path.iterdir() fix * Update test for zipfile.Path.iterdir() * remove whitespace from test file * Rewrite NEWS blurb to describe the user-facing impact and avoid implementation details. * remove redundant [] within set comprehension * Update to use unique_everseen to maintain order and other suggestions in review * remove whitespace and add back add_dirs in tests * Add new standalone function parents using posixpath to get parents of a directory * removing whitespace (sorry) * Remove import pathlib from zipfile.py * Rewrite _parents as a slice on a generator of the ancestry of a path. * Remove check for '.' and '/', now that parents no longer returns those. * Separate calculation of implied dirs from adding those * Re-use _implied_dirs in tests for generating zipfile with dir entries. * Replace three fixtures (abcde, abcdef, abde) with one representative example alpharep. * Simplify implementation of _implied_dirs by collapsing the generation of parent directories for each name.
DinoV
pushed a commit
to DinoV/cpython
that referenced
this pull request
Jan 14, 2020
* fix Path._add_implied_dirs to include all implied directories * fix Path._add_implied_dirs to include all implied directories * Optimize code by using sets instead of lists * 📜🤖 Added by blurb_it. * fix Path._add_implied_dirs to include all implied directories * Optimize code by using sets instead of lists * 📜🤖 Added by blurb_it. * Add tests to zipfile.Path.iterdir() fix * Update test for zipfile.Path.iterdir() * remove whitespace from test file * Rewrite NEWS blurb to describe the user-facing impact and avoid implementation details. * remove redundant [] within set comprehension * Update to use unique_everseen to maintain order and other suggestions in review * remove whitespace and add back add_dirs in tests * Add new standalone function parents using posixpath to get parents of a directory * removing whitespace (sorry) * Remove import pathlib from zipfile.py * Rewrite _parents as a slice on a generator of the ancestry of a path. * Remove check for '.' and '/', now that parents no longer returns those. * Separate calculation of implied dirs from adding those * Re-use _implied_dirs in tests for generating zipfile with dir entries. * Replace three fixtures (abcde, abcdef, abde) with one representative example alpharep. * Simplify implementation of _implied_dirs by collapsing the generation of parent directories for each name.
websurfer5
pushed a commit
to websurfer5/cpython
that referenced
this pull request
Jul 20, 2020
* fix Path._add_implied_dirs to include all implied directories * fix Path._add_implied_dirs to include all implied directories * Optimize code by using sets instead of lists * 📜🤖 Added by blurb_it. * fix Path._add_implied_dirs to include all implied directories * Optimize code by using sets instead of lists * 📜🤖 Added by blurb_it. * Add tests to zipfile.Path.iterdir() fix * Update test for zipfile.Path.iterdir() * remove whitespace from test file * Rewrite NEWS blurb to describe the user-facing impact and avoid implementation details. * remove redundant [] within set comprehension * Update to use unique_everseen to maintain order and other suggestions in review * remove whitespace and add back add_dirs in tests * Add new standalone function parents using posixpath to get parents of a directory * removing whitespace (sorry) * Remove import pathlib from zipfile.py * Rewrite _parents as a slice on a generator of the ancestry of a path. * Remove check for '.' and '/', now that parents no longer returns those. * Separate calculation of implied dirs from adding those * Re-use _implied_dirs in tests for generating zipfile with dir entries. * Replace three fixtures (abcde, abcdef, abde) with one representative example alpharep. * Simplify implementation of _implied_dirs by collapsing the generation of parent directories for each name.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://bugs.python.org/issue37772