Fixes #402 and fixes #53 add ignore_dir_prefix option original credit @nandoflorestan #563
Conversation
…x option original credit @nandoflorestan
…x option original credit @nandoflorestan
Codecov Report
@@ Coverage Diff @@
## master #563 +/- ##
==========================================
- Coverage 90.23% 90.15% -0.09%
==========================================
Files 24 24
Lines 4045 4052 +7
==========================================
+ Hits 3650 3653 +3
- Misses 395 399 +4
Continue to review full report at Codecov.
|
|
@akx I'm a first time contributor, looking to improve my python / open source skills doing a bit of bug fixing. Can you please help with the codecov check issue - I cannot see it showing any error apart from the fact that the test coverage percentage has reduced - obviously, since I added a few lines. I don't know how to add a test for this bugfix or even if it is needed, or if it would increase the percentage enough to pass anyway. Any help appreciated. |
|
This issue for codecov also shows the same problem: https://github.com/codecov/support/issues/135 |
There was a problem hiding this comment.
Thank you for the contribution! I've commented on some possibly problematic bits, and I would love to see some tests for the next iteration. However, I'm a little hesitant to merge this change, because I feel it's not quite enough.
I still think it would be better to generalize the directory filter to be a function that gets called for each dirname (or better yet, also with dirpath, for more advanced use cases), which would default to something like lambda subdir: not (subdir.startswith('.') or subdir.startswith('_')), to retain the current behavior. (The command line API would generate a function like this from the user's input.)
This makes more advanced use cases possible, while still being straightforward to use in a simple (prefix matching, what-have-you) way.
Would you feel up to developing that? :)
| if ignore_dir_prefixes: | ||
| for prefix in ignore_dir_prefixes: | ||
| if subdir.startswith(prefix): | ||
| dirnames.remove(subdir) |
There was a problem hiding this comment.
I don't think it's safe to modify dirnames as it's being iterated over.
This is the reason the original code used a list comprehension to reassign the whole list in one fell swoop.
There was a problem hiding this comment.
Also, once you've removed subdir, you should break out of the loop here; if multiple prefixes match the same subdir, remove will raise ValueError: list.remove(x): x not in list.
| @@ -63,7 +63,8 @@ def _strip(line): | |||
|
|
|||
| def extract_from_dir(dirname=None, method_map=DEFAULT_MAPPING, | |||
| options_map=None, keywords=DEFAULT_KEYWORDS, | |||
| comment_tags=(), callback=None, strip_comment_tags=False): | |||
| comment_tags=(), callback=None, strip_comment_tags=False, | |||
| ignore_dir_prefixes=None): | |||
There was a problem hiding this comment.
ignore_dir_prefixes should probably default to ('.', '_'), to retain the original behavior.
| if not (subdir.startswith('.') or subdir.startswith('_')) | ||
| ] | ||
| for subdir in dirnames: | ||
| if ignore_dir_prefixes: |
There was a problem hiding this comment.
If ignore_dir_prefixes defaults to an empty tuple, this check is unnecessary. Also, it should be done outside the for subdir... loop anyway, as there's no way the prefix list will change during that iteration.
| @@ -313,6 +313,8 @@ class extract_messages(Command): | |||
| 'files or directories with commas(,)'), # TODO: Support repetition of this argument | |||
| ('input-dirs=', None, # TODO (3.x): Remove me. | |||
| 'alias for input-paths (does allow files as well as directories).'), | |||
| ('ignore-dir-prefixes=', None, | |||
| 'Bypass directories whose name start with PREFIX. You can specify this more than once.'), | |||
There was a problem hiding this comment.
How is specifying this more than once handled? All I see is attempting to split a string by ,.
|
@akx Thanks for reviewing. Yes I would like to develop this, and I'll take on board your suggestions. I'm pretty inexperienced as a python dev - so not really sure how to write tests so I'd need some help with that - would you be able to suggest something? |
|
@andy-pi You can look at the tests that Babel currently ships :) With py.test, the test framework we use, the gist of it is that tests are functions named For this feature I'd like to see, at the very least, tests that
You may want to refactor the current code a little for testability; for instance, the HTH! |
|
@akx ok thanks. I will look at it and get to work! |
Babel currently ignores directories prefixed . and _
Variously listed as feature / bug since this behavior is not explicit
To exclude directories now use --ignore_dir_prefix option and specify a prefix, e.g. _
based on the following by @nandoflorestan
nandoflorestan@9d2e0ba#diff-870e23eed5eae4756d0fe90bb4799c81L896