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

Add support for uv init --script #7565

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

tfsingh
Copy link

@tfsingh tfsingh commented Sep 20, 2024

This PR adds support for uv init --script, as defined in issue #7402 (started working on this before I saw jbvsmo's PR). Wanted to highlight a few decisions I made that differ from the existing PR:

  1. --script takes a path, instead of a path/name. This potentially leads to a little ambiguity (I can certainly elaborate in the docs, lmk!), but strictly allowing uv init --script path/to/script.py felt a little more natural than allowing for uv init --script path/to --name script.py (which I also thought would prompt more questions for users, such as should the name include the .py extension?)
  2. The request is processed immediately in the init method, sharing logic in resolving which python version to use with uv add --script. This made more sense to me — since scripts are meant to operate in isolation, they shouldn't consider the context of an encompassing package should one exist (I also think this decision makes the relative codepaths for scripts/packages easier to follow).
  3. No readme — readme felt a little excessive for a script, but I can of course add it in!

@jbvsmo – hope it's okay I'm using the documentation you wrote!


let reporter = PythonDownloadReporter::single(printer);

if script_path.try_exists()? {
Copy link
Author

Choose a reason for hiding this comment

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

Using try_exists here in line with a comment on the previous PR, but should we potentially use fs_err::tokio::metadata so it's async? (Although I don't suspect it's an expensive operation)

@tfsingh tfsingh marked this pull request as ready for review September 20, 2024 01:14
@tfsingh tfsingh marked this pull request as draft September 20, 2024 01:37
@tfsingh tfsingh marked this pull request as ready for review September 20, 2024 01:58
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.

1 participant