Skip to content

feat: integrate minijinja for tasks' command's rendering#3579

Merged
ruben-arts merged 44 commits intoprefix-dev:mainfrom
prsabahrami:feat_minijinja
Apr 17, 2025
Merged

feat: integrate minijinja for tasks' command's rendering#3579
ruben-arts merged 44 commits intoprefix-dev:mainfrom
prsabahrami:feat_minijinja

Conversation

@prsabahrami
Copy link
Copy Markdown
Contributor

This replaces the use of regex with minijnja in order to support template rendering.
I'm not fully confident or in favour of the current design for error handling. So, I'd appreciate it if you could give some feedback on the error handling @Hofer-Julian.
Also, one of the tests are failing because minijinja automatically thinks of {{ python-file }} as a subtraction. I was not able to find the config to play with this. Do you know how to tell minijinja to skip this?

@wolfv
Copy link
Copy Markdown
Member

wolfv commented Apr 12, 2025

The problem with python-file is that it's interpreted as an arithmetic expression (python - file).

I see two options:

  • either we automatically convert python-file to python_file and tell users that's what they should use inside expressions (this is roughly what conda-build / rattler-build do in some instances)
  • We add a "vars" or "global" object that you can index into with strings, such as ${{ vars["python-min"] }} as a longer form for ${{ python-min }}

@Hofer-Julian
Copy link
Copy Markdown
Contributor

That is unfortunate. Trading arithmetic operations for dashes in identifiers is a bad deal for us. It also sucks since we go for kebab-case in all other parts of the manifest.

I think we still want to go with the current approach. Templating makes sense for our use case and using minijinja will open the door for more functionality like boolean, integer or list variables. Other Rust template crates seem to have the same limitation.

Personally, I prefer a third option next to @wolfv's suggestions: error on identifiers with a dash in them. This should make it easy to give good error messages and minimize user confusion.

Copy link
Copy Markdown
Contributor

@Hofer-Julian Hofer-Julian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a few comments.

Let's also error for variables with a dash in them

Copy link
Copy Markdown
Contributor

@Hofer-Julian Hofer-Julian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good first step.

After the comments are addressed, and the rest of the functionality is implemented let's remember to also extend the tests a bit.

@Hofer-Julian
Copy link
Copy Markdown
Contributor

Looks good @prsabahrami!

The tests still need to be updated or there other things missing as well? :)

@Hofer-Julian
Copy link
Copy Markdown
Contributor

I pushed a few small improvements to your branch

@Hofer-Julian
Copy link
Copy Markdown
Contributor

Hofer-Julian commented Apr 16, 2025

Remaining tasks:

  • Use minijinja::Value where it makes sense
  • Give better error message when argument is missing by using miette
  • Add a test that uses a minijinja filter, let's try if title works as expected for example: https://docs.rs/minijinja/latest/minijinja/filters/fn.title.html
  • Extend docs
    • mention that dashes are not allowed in argument names
    • add section about minjinja and give an example with filter

Copy link
Copy Markdown
Contributor

@ruben-arts ruben-arts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed it as a first time user, I was mostly missing examples and a little more documentation on the logic that happens when I define these minijinja things.

I would also like to see more integration level tests to make sure we have a good set of cases that we assure keep working.

This feature feels like a cool first step, the variables and custom functions like being able to reuse global defined variables, being able to get the platform from pixi, being able to use env variables would make this a very useful feature!

Copy link
Copy Markdown
Contributor

@ruben-arts ruben-arts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really promising feature!

@ruben-arts ruben-arts enabled auto-merge (squash) April 17, 2025 13:46
Copy link
Copy Markdown
Contributor

@Hofer-Julian Hofer-Julian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work @prsabahrami, this will enable a lot of exciting use cases in the future 🪄

@Hofer-Julian Hofer-Julian changed the title refactor: integrate minijinja for tasks' command's rendering feat: integrate minijinja for tasks' command's rendering Apr 17, 2025
@ruben-arts ruben-arts merged commit c516779 into prefix-dev:main Apr 17, 2025
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants