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

feat: Show all available task with task list #1286

Merged
merged 26 commits into from
May 1, 2024

Conversation

hoxbro
Copy link
Contributor

@hoxbro hoxbro commented Apr 26, 2024

Resolves #1276

TODO:

  • Add test(s), Skipping it for now
  • Add an example
  • Update schema/model.py.
  • Update reference/configuration.md

Maybe:

  • Add --hidden flag so user can do it from the CLI.

Mainly copied over functionality from other places in the code.

image

@hoxbro hoxbro changed the title Show all available task with task list feat: Show all available task with task list Apr 26, 2024
@ruben-arts
Copy link
Contributor

He @hoxbro, It works like a charm. The change in the list mechanism is ready for me.

The hidden field needs some more work, because it is a manifest change:

  • Add a test to see that it is read correctly in rust.
  • We always try to add manifest features to at least one of the examples/..
  • The schema/model.py needs to know about it.
  • It needs to be documented in the reference/configuration.md
  • Nice to have would be that the pixi task add would get a --hidden flag so user can do it from the cli.

If that would take some time you could also move that to a different PR.

@hoxbro
Copy link
Contributor Author

hoxbro commented Apr 29, 2024

Add a test to see that it is read correctly in rust.

Can you help me and tell me which file(s) I should add the tests to?

We always try to add manifest features to at least one of the examples/..

What do you think about updating the polarify example with the following:

[tasks]
postinstall = "pip install --no-build-isolation --no-deps --disable-pip-version-check -e ."
python_version = {cmd = 'python --version --version', hidden = true}
polar_version = {cmd = 'python -c "import polars; print(f\"Polars version: {polars.__version__}\")"', hidden = true}
start = {depends_on=['python_version', 'polar_version']}
# Before:
# start = 'python -c "import sys; print(f\"Python version: {sys.version}\"); import polars; print(f\"Polars version: {polars.__version__}\")"'

image

The schema/model.py needs to know about it.

I have updated the model and the schema.json file (please correct me if I should not have done this).

It needs to be documented in the reference/configuration.md

This should now be done.

@hoxbro
Copy link
Contributor Author

hoxbro commented Apr 29, 2024

@hoxbro
Copy link
Contributor Author

hoxbro commented Apr 29, 2024

I noticed that env would always be set as {} if cwd and now hidden were set. I updated the code so this is no longer the case in 28cfda. I don't think the code is the pretties, but it works.

 Screenshot 2024-04-29 21 54 02

@ruben-arts
Copy link
Contributor

What do you think about updating the polarify example with the following:

Seeing this I was reminded of this issue: #934
Should this also be included in the hidden logic? To make it like fully hidden?

Copy link
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.

Nice additions!

schema/model.py Outdated Show resolved Hide resolved
@ruben-arts
Copy link
Contributor

The best way to test it in the rust code would be to put it in tests/task_tests.rs but I see that that would be a lot more work then the feature it self. So I think it's find to skip it for now.

@ruben-arts
Copy link
Contributor

For the example, what about the cpp_sdl example where the configure could be hidden? There I could see users using it.

Co-authored-by: Ruben Arts <[email protected]>
@hoxbro
Copy link
Contributor Author

hoxbro commented Apr 30, 2024

Should this also be included in the hidden logic? To make it like fully hidden?

I actually like seeing the commands, even though they are hidden. I don't think adding a silent command will be too hard, but if so, I or someone else can do it in another PR.

I will take a look at the cpp_sdl example later today.

schema/schema.json Outdated Show resolved Hide resolved
@hoxbro hoxbro marked this pull request as ready for review April 30, 2024 13:15
@ruben-arts
Copy link
Contributor

He @hoxbro, I'm really sorry to say this, but we're talking in a voice chat about this feature and @wolfv came with another option which made the logic even simpler, and more intuitive from a user perspective and we would like to change it to that.

Removing the hidden option but use _ in the task name to hide it from the task list.

This would reduce all the changes required to support this feature.

e.g.:

[tasks]
_hidden = "echo This task is not listed but can be run"
echo = {depends_on = ["_hidden"]}
> pixi task list
* echo

@ruben-arts ruben-arts marked this pull request as draft April 30, 2024 13:57
@hoxbro
Copy link
Contributor Author

hoxbro commented Apr 30, 2024

No problem. I will take a look at it later.

Do you also want then want this to be hidden from pixi info?

more intuitive from a user perspective

I think this statement is a bit of a stretch, as this is a hidden feature (pun intended 😄 ) and likely will be harder to discover except by looking at the documentation.

Also, make it a bit weird if you actually want to run the hidden feature.

@ruben-arts
Copy link
Contributor

Do you also want then want this to be hidden from pixi info?

Yes please.

I think this statement is a bit of a stretch, as this is a hidden feature (pun intended 😄 ) and likely will be harder to discover except by looking at the documentation.

I agree that its a stretch as it is purely based on taste. However this feature not resulting in changed behavior of the actual task, which makes me like the small naming trick compared to a full option.

I hope this improve the explanation for the request for change.

src/cli/task.rs Outdated
// Task::Execute(process) => Some(true) != process.hidden,
// _ => true,
// })
.filter(|&(key, _)| !key.as_str().starts_with('_'))
.map(|(key, _)| key)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to use filter_map here as this seems to be the correct thing to do. Just need some more time to get it right.

no spoilers 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% sold on if filter_map is better than the filter and map combination.

@hoxbro hoxbro marked this pull request as ready for review April 30, 2024 17:21
Copy link
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.

Looks great! One tip to consolidate the logic into one function.

src/cli/task.rs Outdated Show resolved Hide resolved
Copy link
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.

Awesome, Thanks!

src/project/environment.rs Outdated Show resolved Hide resolved
@ruben-arts ruben-arts enabled auto-merge (squash) May 1, 2024 07:22
@ruben-arts ruben-arts merged commit f67351e into prefix-dev:main May 1, 2024
27 checks passed
@hoxbro hoxbro deleted the full_task_list branch May 1, 2024 07:34
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.

Add an option to show predefined tasks across environments easily
3 participants