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

Document jinja_custom_elements_names #25

Closed
di opened this issue Sep 8, 2020 · 4 comments · Fixed by #77
Closed

Document jinja_custom_elements_names #25

di opened this issue Sep 8, 2020 · 4 comments · Fixed by #77
Labels
enhancement New feature or request parser This issue relates to Curlylint’s templates parser
Milestone

Comments

@di
Copy link

di commented Sep 8, 2020

Is your proposal related to a problem?

I tried using curlylint on a project that uses Jinja's i18n extension. Without configuring or changing anything, I would get failures like this:

$ cat test.html
{% trans %}
project
{% pluralize %}
projects
{% endtrans %}

$ curlylint test.html
test.html
2:3	Parse error: expected one of 'autoescape', 'block', 'blocktrans', 'comment', 'filter', 'for', 'if', 'ifchanged', 'ifequal', 'ifnotequal', 'not an intermediate Jinja tag name', 'spaceless', 'verbatim', 'with' at 2:3	parse_error

Oh no! 💥 💔 💥
1 error reported

I couldn't find any documentation about configuring support for custom elements.

In order to get this to work, I had to a) read the source code to determine if this was possible b) add the following to my pyproject.toml:

[tool.curlylint]
jinja_custom_elements_names = [['trans', 'pluralize', 'endtrans']]

Describe the solution you’d like

Ideally jinja_custom_elements_names would be documented / officially supported. The format (a list of lists) should also be explained -- it seems that order is important.

Additional context

I also discovered that with a sufficiently large directory of templates, naively adding a string instead of a list, e.g.:

[tool.curlylint]
jinja_custom_elements_names = ['trans']

would result in a significant (exponential?) increase in runtime, and should probably be guarded against.

@di di added the enhancement New feature or request label Sep 8, 2020
@thibaudcolas
Copy link
Owner

thibaudcolas commented Sep 8, 2020

Hey @di 👋 thank you for taking the time to research this and write a detailed report. I’ve intentionally left jinja_custom_elements_names undocumented because it comes straight from jinjalint, on which curlylint is based, and I’ve been unable to decide whether I liked this as an API to extend the parser or not. I usually try to take quite good care not to unnecessarily break things that are documented on a project, and was intending to potentially change how this is configured, hence why it’s been left out.

I think I need more time working on the parser to have an informed opinion of whether this is a good API or not. In the meantime, if people are using this already anyway, it probably does make sense to take the time to document this, even if it did end up changing. In keeping with every other setting, I’ll probably also add a CLI flag for this.


For performance issues, yes, the parser’s performance is rather poor, I think because of it being based on combinators / recursion. Here I suspect it’d iterate through the letters in trans and attempt to generate parsers for {% t %}, {% r %}, etc. I’ll see whether something can be done in this particular case – am also interested in using JSON Schema to both document and validate all of the configuration, but that’s probably for another day.

@thibaudcolas thibaudcolas added the parser This issue relates to Curlylint’s templates parser label Sep 8, 2020
@di
Copy link
Author

di commented Sep 8, 2020

I'd agree that it's not the greatest API -- specifically the need to provide tuples of elements is a bit confusing if you don't understand how the parser works.

That said, being able to add custom elements is a requirement for me to use this library, so I've gone ahead and used jinja_custom_elements_names even though it's undocumented. 🙂

An alternative would be for this library to be able to pick up on the elements that extensions are providing as well, but I feel like that's probably asking a lot!

@thibaudcolas
Copy link
Owner

thibaudcolas commented Apr 25, 2021

Done! This is still not the best API possible, but it’s now officially supported as of v0.13.0. I renamed the option to template_tags in config files, or --template-tags via the CLI.

Note I’ve kept jinja_custom_elements_names for now for backwards compatibility, and in all likelihood it’ll stay around for a while, but I’ll remove it in a future release once people have had a chance to switch over to the supported setting.

Edit: forgot to mention, another change is that there is some basic validation in place that the setting has the expected data structure (list of lists).

@di
Copy link
Author

di commented Apr 25, 2021

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request parser This issue relates to Curlylint’s templates parser
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants