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(cli): add pixi config command #1339

Merged
merged 28 commits into from May 19, 2024

Conversation

chawyehsu
Copy link
Contributor

@chawyehsu chawyehsu commented May 7, 2024

Closes #1022

Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
@wolfv
Copy link
Member

wolfv commented May 7, 2024

Awesome!

* `toml` crate been moved from dev-deps to deps

Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
I found we already have the `toml_edit` crate that can be leveraged,
therefore I think we can drop the `toml` crate.

Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
Old `snake_case` are still supported via serde alias, with this
change the serialized output will always be `kebab-case` though.

Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
@chawyehsu chawyehsu marked this pull request as ready for review May 9, 2024 10:26
@chawyehsu
Copy link
Contributor Author

ready for review

@wolfv
Copy link
Member

wolfv commented May 10, 2024

I just played around with this a little. One thing I found is that a broken value just shows "no config found" instead of showing an error when I run pixi config list. This may be something to improve in the config loading :)

@wolfv
Copy link
Member

wolfv commented May 10, 2024

I also just ran cargo r -- config set change-ps1 false and it would be nice if pixi would print what file it actually changed (e.g. "✅ Added "change-ps1 = true" to your local config at ./.pixi/config.toml".

@wolfv
Copy link
Member

wolfv commented May 10, 2024

I also couldn't figure out if lists work (e.g. cargo r -- config set default-channels 'bioconda,conda-forge,robostack'). Did you try that? Just curious. I know that conda config had some options for append and prepend, but I am not sure if it's used often enough to warrant more complexity. We could also do something like take a comma delimited list or so ...

@wolfv
Copy link
Member

wolfv commented May 10, 2024

Hey @chawyehsu awesome job! I just briefly did some tests on my end for functionality (without looking much at the code). Let me know what you think about the comments :) Looking forward to shipping this! 🚢

@chawyehsu
Copy link
Contributor Author

I found is that a broken value just shows "no config found" instead of showing an error when I run pixi config list. This may be something to improve in the config loading :)

If you mean "No configuration values found" then that's when all the options are default and no user defined ones. A different message could be used here but I'm not sure what's better.

it would be nice if pixi would print what file it actually changed

Sounds good, will add.

I also couldn't figure out if lists work (e.g. cargo r -- config set default-channels 'bioconda,conda-forge,robostack'). Did you try that? Just curious.

For default-channels, the only valid input format would be something like '["bioconda","conda-forge","robostack"]' which looks tedious but it's a strictly serialized list.

I am not sure if it's used often enough.

Me neither. For me I would prefer pixi config edit rather than pixi config set for editing complex options. And I can't find a friendly way to support editing object type of options like [mirrors], imagine pixi config set mirrors '<input>', what should <input> look like?

had some options for append and prepend

Do they support list type of options only?

Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
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 work already! Left some comments.

src/config.rs Outdated
})
.transpose()?;
}
_ => return Err(miette::miette!("unsupported key: {}", key)),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be really nice if the user would get a list of options printed, I personally was making a stupid typo and it made me look into the code how it should be.

Also do you have any idea's how we could test if we support all options?

src/config.rs Outdated Show resolved Hide resolved
src/cli/config.rs Outdated Show resolved Hide resolved
src/cli/config.rs Outdated Show resolved Hide resolved
Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
without requiring the `--global` flag.

Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
group by top-level keys when doing pattern matching

Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
chawyehsu and others added 2 commits May 15, 2024 08:54
Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
@ruben-arts
Copy link
Contributor

He @chawyehsu,

I've been using it now for some time and it feels great.

Could you add documentation? And possible some examples to the --help. E.g.:

> pixi config set -h
Set a configuration value

Example:
	$ pixi config set --global default-channels '["conda-forge", "nvidia"]'

If that's in I would be happy to merge it!

Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
@chawyehsu
Copy link
Contributor Author

Doc and test have been added. Also, two new subcommands mentioned above prepend and append are added for easier altering list keys.

@ruben-arts
Copy link
Contributor

Thanks a lot @chawyehsu, this looks good to me!

@ruben-arts ruben-arts added enhancement New feature or request cli Issue related to CLI configuration Issue related to configuration of project or global behavior labels May 19, 2024
@ruben-arts ruben-arts merged commit 4c1cc5d into prefix-dev:main May 19, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Issue related to CLI configuration Issue related to configuration of project or global behavior enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement a CLI for the configuration
3 participants