bpo-31920: Fix pygettext directory walk by insolite · Pull Request #4225

bpo-31920: Fix pygettext directory walk by insolite · Pull Request #4225 · python/cpython

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!

Choose a reason for hiding this comment

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

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:

Choose a reason for hiding this comment

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

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:

Choose a reason for hiding this comment

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

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)

Choose a reason for hiding this comment

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

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 Misc/NEWS.d/next/Tools-Demos directory (don't forget to add "Patch by Oleg Krasnikov.") and add your name in Misc/ACKS.

3.4 and 3.5 are only accepting security fixes now.

Choose a reason for hiding this comment

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

Add a news entry, your name in Misc/NEWS, and additional tests.

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Tests have been updated in #6259.