Skip to content

feat: argument variables in tasks#3433

Merged
Hofer-Julian merged 30 commits intomainfrom
feat_taskargs
Apr 3, 2025
Merged

feat: argument variables in tasks#3433
Hofer-Julian merged 30 commits intomainfrom
feat_taskargs

Conversation

@prsabahrami
Copy link
Copy Markdown
Contributor

Adds support for arguments passed into the task commands

@prsabahrami prsabahrami changed the title feat: argument variables in tasks wip: argument variables in tasks Mar 25, 2025
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.

Had a quick glance at the tests and left a few comments. Will try the code changes tomorrow. Would be good to solve the merge conflicts until then. Not sure what's up with the trampoline, but, I think you can just throw away your trampoline changes and take the ones from main

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.

Had another quick look and added a few nitpicks

@prsabahrami prsabahrami marked this pull request as ready for review March 27, 2025 16:26
@prsabahrami prsabahrami changed the title wip: argument variables in tasks feat: argument variables in tasks Mar 27, 2025
@Hofer-Julian
Copy link
Copy Markdown
Contributor

@prsabahrami I force pushed to your branch to get rid of the unnecessary trampoline changes

@Hofer-Julian
Copy link
Copy Markdown
Contributor

Hofer-Julian commented Mar 28, 2025

Things to do before we can merge this:

  • Update schema/model.py and run pixi run generate-schema
  • Extend docs/workspace/advanced_tasks.md

chore: raise an error if a there is an argument with no default after one with a default

chore: add support for running tasks with arguments passed via cli

chore: add support for running tasks with arguments passed via depends

fix: the serialization of dependencies as tables in tasks

Update crates/pixi_manifest/src/task.rs

Co-authored-by: Hofer-Julian <30049909+Hofer-Julian@users.noreply.github.com>

chore: remove unused struct `DisplayDependency`

chore: change  to a struct

fix: use  library and a dictionary to create the task argument tests

chore: remove extra method from Dependency

chore: add pre-commit-minimal back (#3449)

refactor: rename variables for clarity in task argument tests

fix: add the trampoline binary back

update trampoline binaries

[CI]: Update trampoline binaries for all targets

Remove unneeded implementation

Update src/task/task_graph.rs

Co-authored-by: Tim de Jager <tim@prefix.dev>
@ruben-arts
Copy link
Copy Markdown
Contributor

ruben-arts commented Mar 28, 2025

Hey @prsabahrami, already great work!

I'm finding some UX things that I would like to improve:

Screenshot 2025-03-28 at 16 26 49
In this example could you make the blue task rendered with the argument.

Screenshot 2025-03-28 at 16 26 39
Could you make the error more readable, using colors would be awesome, but the way it's formatted currently makes it hard to read.

Screenshot 2025-03-28 at 16 32 21
Could you extend this error with what it expected vs what it got, possibly with a help to quote the input to make it fit into one.

Screenshot 2025-03-28 at 16 35 04
Please make sure that it's documented well that with an argument and without acts differently with the trailing commands.

@ruben-arts
Copy link
Copy Markdown
Contributor

Could you add an example that uses this feature to the examples/

@prsabahrami
Copy link
Copy Markdown
Contributor Author

@Hofer-Julian ready for your review!

@prsabahrami
Copy link
Copy Markdown
Contributor Author

image
This is how the errors are being shown right now. Let me know if you have any comments!

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.

A few comments to improve the docs

@Hofer-Julian
Copy link
Copy Markdown
Contributor

One other thing, I remember: you added the logic to deal with this case, right?

A depends on B and D
B depends on C with args 1
D depends on D with args 2

Do we have tests for that?
If not, let's add a couple of scenarios here!

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.

A few code comments

prsabahrami and others added 5 commits April 2, 2025 09:46
Co-authored-by: Hofer-Julian <30049909+Hofer-Julian@users.noreply.github.com>
Co-authored-by: Hofer-Julian <30049909+Hofer-Julian@users.noreply.github.com>
@prsabahrami prsabahrami requested a review from Hofer-Julian April 2, 2025 15:18
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 🚀

I tweaked docs and error messages a bit. Let's bring it in!

@Hofer-Julian Hofer-Julian enabled auto-merge (squash) April 3, 2025 07:57
@Hofer-Julian Hofer-Julian merged commit 76369f2 into main Apr 3, 2025
33 checks passed
@Hofer-Julian Hofer-Julian deleted the feat_taskargs branch April 3, 2025 08:02
@bollwyvl
Copy link
Copy Markdown
Contributor

bollwyvl commented Apr 7, 2025

Are {{ arg }} values available in inputs and outputs?

A quick look through the code suggests no... will make a separate issue.

@Hofer-Julian
Copy link
Copy Markdown
Contributor

A quick look through the code suggests no... will make a separate issue.

Yeah, please do so, we are open to adding argument expansion to other fields where it makes sense

@bollwyvl
Copy link
Copy Markdown
Contributor

bollwyvl commented Apr 7, 2025

#3533

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.

6 participants