- Notifications
You must be signed in to change notification settings - Fork 463
Initial support for reading mapping configuration as TOML#1108
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
akx commented Aug 1, 2024
codecovbot commented Aug 1, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## master #1108 +/- ## ========================================== + Coverage 91.21% 91.22% +0.01% ========================================== Files 27 27 Lines 4496 4561 +65 ========================================== + Hits 4101 4161 +60 - Misses 395 400 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
akx commented Aug 1, 2024
tomasr8 commented Aug 1, 2024
Looks great! I was just wondering how you can specify ignore paths. Would this work? [[babel.mappings]] method = "ignore"pattern = "test/*"Can the pattern be a list? |
Uh oh!
There was an error while loading. Please reload this page.
akx commented Aug 2, 2024
By not specifying them in a mapping at all... 😅 There's also the command-line But if your imagined use-case is "extract from all HTML files with
Might as well support that too! |
tomasr8 left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if your imagined use-case is "extract from all HTML files with jinja2, but never extract anything from test*.html", that would be a separate issue. Perfectly doable with a no-op extractor (that we don't yet have)
I'm a bit tired so maybe I missed something, but there already exists a noop extractor - extract_nothing so for example this cfg currently works as expected:
[ignore:**/*_test.py] [python:**.py]My previous question was about whether this will keep working with toml and I just confimed that it does i.e. this works:
[[babel.mappings]] method = "ignore"pattern = "**/*_test.py" [[babel.mappings]] method = "python"pattern = "**/*.py"Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
AA-Turner left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Entirely unsolicited, but an option might be:
[extractors] custom = "mypackage.module:myfunc"# Python source files [[mappings.python]] pattern = "**.py"# Genshi templates [[mappings.genshi]] pattern = "**/templates/**.html"include_attrs = "" [[mappings.genshi]] pattern = "**/templates/**.txt"template_class = "genshi.template:TextTemplate"encoding = "latin-1"# Some custom extractor [[mappings."customextractor"]] pattern = "**/custom/*.*"This makes it clearer when scanning the file which table is for which method -- something the current .cfg file does well at. I would also drop the babel. prefix for the babel.toml file -- it is needlessly explicit (see e.g. .ruff.toml which drops the full tool.ruff prefix).
A
Uh oh!
There was an error while loading. Please reload this page.
akx commented Aug 3, 2024
My bad, it's not you being tired. I had forgotten there's a special |
akx commented Aug 3, 2024
Not at all, thank you for the input! Much appreciated.
I'm not completely sold on that – feels like it becomes too easy to accidentally do
That's probably a good idea. |
AA-Turner commented Aug 3, 2024
I think this is a problem regardless. For example this PR adds a config file with one entry, [mapping] method = "jinja2"pattern = "**.html"which looks right, but would instantly fail. Worse would be: [babel.mappings] method = "genshi"pattern = "**/templates/**.html"include_attrs = "" [babel.mappings] method = "genshi"pattern = "**/templates/**.txt"template_class = "genshi.template:TextTemplate"encoding = "latin-1"This is luckily an error in TOML, but still confusing. There should be a clear and early error message if I think with the clear error message (that I posit is needed regardless of the option taken), my proposal still has merit. However, it does have drawbacks as you note. |
Co-authored-by: Adam Turner <[email protected]>
fc25622 to fbd0c5dCompareakx commented Aug 7, 2024
@AA-Turner@tomasr8 Do you have the time to take another look at this? Thanks! |
tomasr8 left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me :) Should we add some sample toml configs to the docs as well?
AA-Turner left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I moved theme.conf to theme.toml in Sphinx, I added a conversion helper (python -m sphinx.theming conf_to_toml <path>) for consumers to use. Perhaps something similar could be helpful here, if so I could investigate a PR?
A
Uh oh!
There was an error while loading. Please reload this page.
akx commented Aug 7, 2024
Thank you for the wonderful comments and review! ❤️ |
Babel >= 2.16.0 (from 2024-08) supports this. python-babel/babel#1108
This PR offers to implement part of #777.
Namely, following this PR you would be able to run
pybabel extract -F babel.toml– or evenpybabel extract -F pyproject.toml, as we already will supporttool.babelas well asbabelfor the namespace. (Auto-detecting apyproject.tomlinstead ofsetup.cfgorbabel.cfgis not implemented in this PR.)I'm requesting comments on the proposed TOML format, which borrows the idea of mappings-as-lists from Mypy's overrides configuration.