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

Expose toolchain-preference as a CLI and configuration file option #4424

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Jun 20, 2024

Exposes the option added in #4416. Adds --toolchain-preference and tool.uv.toolchain-preference to configure if system or managed toolchains are preferred. Users can opt-out of managed toolchains or system toolchains entirely as well.

@zanieb zanieb added the preview Experimental behavior label Jun 20, 2024
@zanieb zanieb force-pushed the zb/toolchain-setting branch 4 times, most recently from bd14d24 to c64a2c8 Compare June 20, 2024 16:19
@zanieb zanieb marked this pull request as ready for review June 20, 2024 16:23
@zanieb
Copy link
Member Author

zanieb commented Jun 20, 2024

Should we bikeshed the name? Should it just be --toolchains?

Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

LGTM.

As for the name... Is this something that you think will be commonly used on the CLI? If so, I think I'd favor a shorter name. Otherwise, I like the descriptiveness of --toolchain-preference.

One other possible downside is redundancy here. Namely, --toolchain-preference prefer-system has "prefer" twice in it, but arguably is just as clear if "prefer" only appeared once. Not quite sure what the right answer is there.

@zanieb
Copy link
Member Author

zanieb commented Jun 20, 2024

One other possible downside is redundancy here. Namely, --toolchain-preference prefer-system has "prefer" twice in it, but arguably is just as clear if "prefer" only appeared once.

Agree, this is a source of discomfort for me but idk it's also very clear.

I don't expect this to be used much from the CLI — mostly as a persistent configuration option.

@zanieb
Copy link
Member Author

zanieb commented Jun 20, 2024

I'm fine adjusting this later if we need to since it's in preview.

Base automatically changed from zb/toolchain-environment to main June 20, 2024 17:54
@zanieb
Copy link
Member Author

zanieb commented Jun 20, 2024

I guess another option is I drop the prefer prefix so we'd have

toolchain-preference = managed | system | only-managed | only-system | installed-managed

which is pretty reasonable too?

@BurntSushi
Copy link
Member

I guess another option is I drop the prefer prefix so we'd have

toolchain-preference = managed | system | only-managed | only-system | installed-managed

which is pretty reasonable too?

I think that's probably okay. I thought of that too, but wondered whether it might leave folks wondering the difference between managed and only-managed.

@zanieb zanieb merged commit 93c6e0d into main Jun 20, 2024
47 checks passed
@zanieb zanieb deleted the zb/toolchain-setting branch June 20, 2024 18:42
zanieb added a commit that referenced this pull request Jul 2, 2024
I think `--toolchain-preference system` is sufficiently clear and
`--toolchain-preference prefer-system` is excessively verbose. This was
discussed in the original pull request at
#4424 but because we had a case for
preferring "installed managed" toolchains I was hesitant to change it.
Now that I've dropped that in #4601, I think we can drop the prefix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Experimental behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants