Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed in-place editing of dir list while iterating #476

Closed
wants to merge 0 commits into from

Conversation

MarcDufresne
Copy link
Contributor

I was having an issue where iterating the following list of directories would still have directories with . at the beginning (seems to relate to #471)

All that was causing babel to find messages in the .tox directory which should be ignored.

Input:

[
    '.git', 
    '.idea', 
    '.tox', 
    '.zanata-cache', 
    'supervisor', 
    'tests', 
    'userdoc',
    'userdoc.egg-info'
]

Output:

[
    '.idea', 
    '.zanata-cache', 
    'supervisor', 
    'tests', 
    'userdoc', 
    'userdoc.egg-info'
]

So as you can see only two directories have been removed. (Happens because changing the list while iterating over it messes up the iterator, thus causing skips in the list)

My solution creates a copy of the list and changes the copy while iterating over the original. Once iteration is over we assign the value on the copy to the original.

Output with this patch:

[
    'supervisor', 
    'tests', 
    'userdoc', 
    'userdoc.egg-info'
]

We can clearly see that all directories starting with . have been removed and the function now works as intended.

@akx
Copy link
Member

akx commented Mar 10, 2017

I don't believe this works correctly.

The documentation for os.walk explicitly says that one should modify dirnames in-place (emphasis mine):

When topdown is True, the caller can modify the dirnames list in-place (perhaps using del or slice assignment), and walk() will only recurse into the subdirectories whose names remain in dirnames; [...]

The line

dirnames = filtered_dirnames

does not modify the original object the name dirnames points to; it reassigns another object to that name, and the os.walk generator does not have knowledge of that new object.

A simpler patch that should achieve the same effect is replacing

for subdir in dirnames:

with

for subdir in dirnames[:]:

i.e. grabbing a copy of dirnames just for the filter iteration.

Though, honestly, a more Pythonic way, I guess, would be to replace the for loop with a list comprehension (note the [:] slice assignment, which edits the original dirnames object):

dirnames[:] = [
  subdir
  for subdir
  in dirnames
  if not (subdir.startswith('.') or subdir.startswith('_'))
]

Copy link
Member

@akx akx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(See comment in PR.)

@codecov-io
Copy link

Codecov Report

Merging #476 into master will decrease coverage by 1.36%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #476      +/-   ##
==========================================
- Coverage   90.14%   88.78%   -1.37%     
==========================================
  Files          24       24              
  Lines        3979     3977       -2     
==========================================
- Hits         3587     3531      -56     
- Misses        392      446      +54
Impacted Files Coverage Δ
babel/messages/extract.py 94.11% <100%> (-0.78%)
babel/_compat.py 53.48% <0%> (-46.52%)
babel/localtime/_unix.py 11.53% <0%> (-20.52%)
babel/localtime/_win32.py 59.25% <0%> (-3.71%)
babel/localtime/init.py 62.5% <0%> (-2.5%)
babel/messages/catalog.py 92.39% <0%> (-2.34%)
babel/util.py 90.47% <0%> (-1.37%)
babel/localedata.py 94.33% <0%> (-0.95%)
babel/support.py 82.28% <0%> (-0.4%)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f0d13b...dc897fc. Read the comment docs.

@MarcDufresne
Copy link
Contributor Author

Any updates on this guys? We're using a workaround at the moment but it would be great if it was fixed in the upstream :)

@akx
Copy link
Member

akx commented Apr 10, 2017

Ahhh, sorry @MarcDufresne, this got limboed due to Travis being silly. I re-restarted the build now, let's hope it can manage to post the result correctly so I can merge without having to invoke admin privileges...

@akx
Copy link
Member

akx commented Apr 10, 2017

... Well, that didn't work like it should have. Let me recreate the PR for you, @MarcDufresne. Sorry!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants