Skip to content
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

Merged
merged 13 commits into from
Aug 7, 2024
Merged

Conversation

akx
Copy link
Member

@akx akx commented Aug 1, 2024

This PR offers to implement part of #777.

Namely, following this PR you would be able to run pybabel extract -F babel.toml – or even pybabel extract -F pyproject.toml, as we already will support tool.babel as well as babel for the namespace. (Auto-detecting a pyproject.toml instead of setup.cfg or babel.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.

[babel.extractors]
custom = "mypackage.module:myfunc"

# Python source files
[[babel.mappings]]
method = "python"
pattern = "**.py"

# Genshi templates
[[babel.mappings]]
method = "genshi"
pattern = "**/templates/**.html"
include_attrs = ""

[[babel.mappings]]
method = "genshi"
pattern = "**/templates/**.txt"
template_class = "genshi.template:TextTemplate"
encoding = "latin-1"

# Some custom extractor
[[babel.mappings]]
method = "custom"
pattern = "**/custom/*.*"

@akx
Copy link
Member Author

akx commented Aug 1, 2024

cc @Zharktas, @tomasr8, @eemeli – please weigh in if you have ideas :)

Copy link

codecov bot commented Aug 1, 2024

Codecov Report

Attention: Patch coverage is 91.30435% with 6 lines in your changes missing coverage. Please review.

Project coverage is 91.22%. Comparing base (d3346ee) to head (a820cf5).
Report is 2 commits behind head on master.

Files Patch % Lines
babel/messages/frontend.py 91.30% 6 Missing ⚠️
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     
Flag Coverage Δ
macos-12-3.10 90.02% <91.30%> (+0.03%) ⬆️
macos-12-3.11 89.95% <86.95%> (-0.04%) ⬇️
macos-12-3.12 90.13% <86.95%> (-0.04%) ⬇️
macos-12-3.13-dev 89.65% <86.95%> (-0.03%) ⬇️
macos-12-3.8 89.95% <91.30%> (+0.03%) ⬆️
macos-12-3.9 89.95% <91.30%> (+0.03%) ⬆️
macos-12-pypy3.10 ?
ubuntu-22.04-3.10 90.04% <91.30%> (+0.03%) ⬆️
ubuntu-22.04-3.11 89.98% <86.95%> (-0.04%) ⬇️
ubuntu-22.04-3.12 90.15% <86.95%> (-0.04%) ⬇️
ubuntu-22.04-3.13-dev 89.67% <86.95%> (-0.03%) ⬇️
ubuntu-22.04-3.8 89.97% <91.30%> (+0.03%) ⬆️
ubuntu-22.04-3.9 89.97% <91.30%> (+0.03%) ⬆️
ubuntu-22.04-pypy3.10 90.04% <91.30%> (+0.03%) ⬆️
windows-2022-3.10 90.16% <91.30%> (+0.03%) ⬆️
windows-2022-3.11 90.10% <86.95%> (-0.04%) ⬇️
windows-2022-3.12 90.27% <86.95%> (-0.04%) ⬇️
windows-2022-3.13-dev 89.79% <86.95%> (-0.04%) ⬇️
windows-2022-3.8 90.09% <91.30%> (+0.03%) ⬆️
windows-2022-3.9 90.09% <91.30%> (+0.03%) ⬆️
windows-2022-pypy3.10 90.16% <91.30%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@akx
Copy link
Member Author

akx commented Aug 1, 2024

Oh, and cc @miigotu too of course as the opener of #777 😄

@tomasr8
Copy link
Member

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?

babel/messages/frontend.py Outdated Show resolved Hide resolved
@akx
Copy link
Member Author

akx commented Aug 2, 2024

Looks great! I was just wondering how you can specify ignore paths.

By not specifying them in a mapping at all... 😅 There's also the command-line --ignore-dirs option for wholesale directory ignoring.

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), since the list of mappings is in priority order, but that'd also work for .cfg style mapping files.

Can the pattern be a list?

Might as well support that too!

@akx akx requested a review from eemeli August 2, 2024 05:13
@akx akx marked this pull request as ready for review August 2, 2024 12:46
@akx akx changed the title [RFC/WIP] Initial support for reading configuration as TOML Initial support for reading configuration as TOML Aug 2, 2024
@akx akx added this to the Babel 2.16 milestone Aug 2, 2024
@akx akx requested a review from tomasr8 August 2, 2024 12:46
Copy link
Member

@tomasr8 tomasr8 left a 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"

babel/messages/frontend.py Outdated Show resolved Hide resolved
babel/messages/frontend.py Outdated Show resolved Hide resolved
Copy link
Contributor

@AA-Turner AA-Turner left a 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

babel/messages/frontend.py Outdated Show resolved Hide resolved
@akx
Copy link
Member Author

akx commented Aug 3, 2024

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:

My bad, it's not you being tired. I had forgotten there's a special if method == 'ignore': case in extract_from_file that handles that (as well as the 'ignore': extract_nothing case in _BUILTIN_EXTRACTORS)...

@akx
Copy link
Member Author

akx commented Aug 3, 2024

Entirely unsolicited,

Not at all, thank you for the input! Much appreciated.

but an option might be: ...

I'm not completely sold on that – feels like it becomes too easy to accidentally do [mappings.python] if you expect to only have a single mapping for python, and you'd then get a (possibly inscrutable) parsing type error.

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).

That's probably a good idea.

@AA-Turner
Copy link
Contributor

I'm not completely sold on that – feels like it becomes too easy to accidentally do [mappings.python] if you expect to only have a single mapping for python, and you'd then get a (possibly inscrutable) parsing type error.

I think this is a problem regardless. For example this PR adds a config file with one entry, [jinja2: **.html]. Someone without knowledge of TOML's quirks might write

[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 isinstance(config['mapping'], dict) is True (or in my proposal, any(isinstance(k, dict) for k in config['mapping'].keys())).

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.

@akx akx force-pushed the toml-support-1 branch 2 times, most recently from fc25622 to fbd0c5d Compare August 5, 2024 15:50
@akx akx changed the title Initial support for reading configuration as TOML Initial support for reading mapping configuration as TOML Aug 5, 2024
@akx
Copy link
Member Author

akx commented Aug 7, 2024

@AA-Turner @tomasr8 Do you have the time to take another look at this? Thanks!

Copy link
Member

@tomasr8 tomasr8 left a 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?

Copy link
Contributor

@AA-Turner AA-Turner left a 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

babel/messages/frontend.py Outdated Show resolved Hide resolved
@akx akx merged commit d26a669 into master Aug 7, 2024
26 checks passed
@akx
Copy link
Member Author

akx commented Aug 7, 2024

Thank you for the wonderful comments and review! ❤️

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.

4 participants