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 find_uv_bin and declare typing support #1728

Merged
merged 1 commit into from
Feb 20, 2024
Merged

Expose find_uv_bin and declare typing support #1728

merged 1 commit into from
Feb 20, 2024

Conversation

gaborbernat
Copy link
Contributor

@gaborbernat gaborbernat commented Feb 20, 2024

Resolves #1677

Signed-off-by: Bernát Gábor <bgabor8@bloomberg.net>

def detect_virtualenv() -> str:

def _detect_virtualenv() -> str:
Copy link
Contributor Author

@gaborbernat gaborbernat Feb 20, 2024

Choose a reason for hiding this comment

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

Marked this as private explicitly 👍 because by default in Python anything that's no prohibited (at least by convention in case of _) is allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have tests for these files? I don't see any Python test harness just yet 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Nope. We should probably pip install uv and python -m uv smoke test in CI when we build wheels for releases? We could track this separately.

Copy link
Member

Choose a reason for hiding this comment

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

We could also probably install directly e.g. uv pip install -e ./python/uv and then test in the "Smoke test" step?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, agreed, though likely should be a separate PR 😊 Up to you if you want to wait for this until that lands.

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to put that up if you're interested! :) unless we get to it first

@zanieb
Copy link
Member

zanieb commented Feb 20, 2024

Thanks for contributing!

This is a duplicate of #1685 but there were some problems there and this looks correct to me.



if __name__ == "__main__":
def _run() -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By moving such logic into a private _run method, we avoid exposing all global variables as publicly import-able by anyone.

@zanieb zanieb added the enhancement New feature or improvement to existing functionality label Feb 20, 2024
@sbrugman sbrugman mentioned this pull request Feb 20, 2024
@zanieb zanieb merged commit 469ccf8 into astral-sh:main Feb 20, 2024
7 checks passed
@gaborbernat gaborbernat deleted the expose-find-uv-bin-with-types branch February 20, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No type annotations for find_uv_bin
2 participants