Skip to content

gh-130197: Fix (and test) option in pygettext #133021

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

StanFromIreland
Copy link
Contributor

@StanFromIreland StanFromIreland commented Apr 26, 2025

This also required some updating of the snapshot testing logic to handle multiple files. This function should get more testing and I will come back to this once I complete more research. For now I fixed (and tested) what was implemented.

@StanFromIreland
Copy link
Contributor Author

Request @tomasr8 @serhiy-storchaka

@tomasr8 tomasr8 self-requested a review April 26, 2025 18:48
@StanFromIreland
Copy link
Contributor Author

StanFromIreland commented Apr 26, 2025

Also, can we rename it to skip-docstrings since it just make so much more sense. (There so be no backwards compatibility concerns since the current option dosen't work anyway)

edit: Tommorow I’ll update the PR renaming it to exclude-docstrings

edit2: It has now been renamed to exclude-docstrings. This is much clearer (and consistent) that the option needs to be passed something, and is less conflicting with --docstrings.

@StanFromIreland StanFromIreland requested a review from tomasr8 April 27, 2025 15:09
@tomasr8
Copy link
Member

tomasr8 commented Apr 27, 2025

ok if we're gonna go with fnmatch, I'd also update the docs for the option to mention that fnmatch-style patterns are supported. I'd also add more tests for wildcards and directories.

@StanFromIreland
Copy link
Contributor Author

ok if we're gonna go with fnmatch, I'd also update the docs for the option to mention that fnmatch-style patterns are supported. I'd also add more tests for wildcards and directories.

Do we want this though? I guess we can, but should we.

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.

2 participants