-
Notifications
You must be signed in to change notification settings - Fork 447
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
Initial support for reading mapping configuration as TOML #1108
Conversation
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. |
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? |
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! |
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"
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."custom extractor"]]
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
My bad, it's not you being tired. I had forgotten there's a special |
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. |
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
fbd0c5d
Compare
@AA-Turner @tomasr8 Do you have the time to take another look at this? Thanks! |
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?
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
Thank you for the wonderful comments and review! ❤️ |
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.babel
as well asbabel
for the namespace. (Auto-detecting apyproject.toml
instead ofsetup.cfg
orbabel.cfg
is 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.