fix: preserve members order in ExtractorProcessor.__call__()#537
Open
terminalchai wants to merge 1 commit into
Open
fix: preserve members order in ExtractorProcessor.__call__()#537terminalchai wants to merge 1 commit into
terminalchai wants to merge 1 commit into
Conversation
When the 'members' argument is supplied to Unzip or Untar, the returned list
of extracted file paths now follows the same order as 'members' instead of
the arbitrary order produced by os.walk().
Previously the code walked the extract directory and appended files whenever
any member prefix matched, meaning the order of results was determined by
the filesystem rather than the caller.
The fix replaces the single os.walk() loop with two code paths:
- members is None -> walk and collect all files (behaviour unchanged)
- members provided -> walk once into a dict {member: full_path}, then
return [dict[m] for m in self.members] to guarantee caller-specified order.
Walking only once preserves O(N) complexity.
Add test_unpacking_members_order_preserved for both Unzip and Untar.
Closes fatiando#457
|
I traced the two failing From the CI logs:
A small way to fix both without changing behavior is to move the two extracted-file listing paths into private helpers and rename diff --git a/pooch/processors.py b/pooch/processors.py
@@
def _extract_file(self, fname, extract_dir):
"""
This method receives an argument for the archive to extract and the
destination path.
MUST BE IMPLEMENTED BY CHILD CLASSES.
"""
+ def _all_extracted_filenames(self):
+ fnames = []
+ for path, _, files in os.walk(self.extract_dir):
+ for filename in files:
+ fnames.append(os.path.join(path, filename))
+ return fnames
+
+ def _filter_extracted_members(self):
+ extracted = {}
+ for path, _, files in os.walk(self.extract_dir):
+ for filename in files:
+ relpath = os.path.normpath(
+ os.path.join(os.path.relpath(path, self.extract_dir), filename)
+ )
+ for member in self.members:
+ if relpath.startswith(os.path.normpath(member)):
+ extracted[member] = os.path.join(path, filename)
+ return [extracted[member] for member in self.members if member in extracted]
+
def __call__(self, fname, action, pooch):
@@
# of unzipped files, filtered by the given members list
if self.members is None:
# No filter: collect all extracted files in walk order
- fnames = []
- for path, _, files in os.walk(self.extract_dir):
- for filename in files:
- fnames.append(os.path.join(path, filename))
+ fnames = self._all_extracted_filenames()
else:
# Build a mapping from each requested member to its extracted path.
# Walking the directory only once keeps this O(N) in the number of
# extracted files regardless of how many members were requested.
- extracted = {}
- for path, _, files in os.walk(self.extract_dir):
- for filename in files:
- relpath = os.path.normpath(
- os.path.join(
- os.path.relpath(path, self.extract_dir), filename
- )
- )
- for m in self.members:
- if relpath.startswith(os.path.normpath(m)):
- extracted[m] = os.path.join(path, filename)
# Return files in the same order as self.members so callers can
# rely on the position of each file in the returned list.
- fnames = [extracted[m] for m in self.members if m in extracted]
+ fnames = self._filter_extracted_members()I verified this locally with: PYTHONPATH=/tmp/pooch-style python -m black --check pooch doc
PYTHONPATH=/tmp/pooch-style python -m flake8 pooch doc
PYTHONPATH=/tmp/pooch-style python -m pylint pooch/processors.py
PATH=/tmp/pooch-style/bin:$PATH PYTHONPATH=/tmp/pooch-style burocrata --check --extension=py pooch doc
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 PYTHONPATH=. pytest pooch/tests/test_processors.pyThe processor tests pass: One note: resolving the unpinned style deps today gives |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
Closes #457.
When \members\ is supplied to \Unzip\ or \Untar, the returned list of file paths now follows the *same order as \members* instead of the arbitrary order produced by \os.walk().
Root cause
The previous implementation iterated \os.walk(extract_dir)\ and appended a file path whenever any member prefix matched. The walk order is filesystem- and OS-dependent, making the resulting list order unpredictable and unrelated to the caller-specified \members.
Fix
Replaced the single \os.walk\ loop with two code paths:
Walking the directory only once preserves the existing O(N) complexity (N = number of extracted files), as suggested by @santisoler in the issue.
Changes
Test
34 passed, 7 deselected in 25.30sBoth \Unzip\ and \Untar\ variants of the new test confirm that requesting members in forward then reverse order produces correspondingly ordered results.