bpo-31920: Fix pygettext directory walk #4225
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. Thanks again to your contribution and we look forward to looking at it! |
There was a problem hiding this comment.
LGTM, but please test also bugs of your initial version. This can be easy done with the same test method.
-
Add the CVS directory and put a Python file with an unique message in it. This message shouldn't be found in the result file.
-
Create a directory with the '.py' extension. pygettext should not fail trying to open it.
| source_dir = 'pypkg' | ||
| text = 'Text to translate' | ||
| with temp_cwd(None) as cwd: | ||
| with temp_dir(os.path.join(cwd, source_dir)) as sdir: |
There was a problem hiding this comment.
I think it is better to use temp_dir(None) and test with a path that is not a child of the current directory. If pygettext.py ignores the argument and searches from the current directory, it will pass the current test, but not the test with independent test directory.
| text = 'Text to translate' | ||
| with temp_cwd(None) as cwd: | ||
| with temp_dir(os.path.join(cwd, source_dir)) as sdir: | ||
| with open(os.path.join(sdir, 'pymod.py'), 'w') as sfile: |
There was a problem hiding this comment.
Would be nice to put the test file deeper in the directory tree, e.g. in os.path.join(sdir, 'pypkg', 'pymod.py'). This would test that directories are searched recursively.
| data = fp.read() | ||
| msgids = re.findall(r'msgid "(.*?)"', data) | ||
|
|
||
| self.assertIn(text, msgids) |
There was a problem hiding this comment.
Wouldn't be simpler to test the containment directly in the file content?
self.assertIn('msgid "%s"' % text, data)|
Please add a news entry in the |
|
3.4 and 3.5 are only accepting security fixes now. |
|
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 |
|
Tests have been updated in #6259. |
Can be backported to >=3.4 I think.
https://bugs.python.org/issue31920