Skip to content

Refactor wheel_resolver to allow for non-PyPI downloads#167

Merged
tm-jdelapuente merged 4 commits intomasterfrom
allow-non-PyPI-downloads
Aug 20, 2024
Merged

Refactor wheel_resolver to allow for non-PyPI downloads#167
tm-jdelapuente merged 4 commits intomasterfrom
allow-non-PyPI-downloads

Conversation

@tm-jdelapuente
Copy link
Contributor

A pending problem (see this comment) was to allow downloads outside PyPI.

This PR refactors wheel_resolver to allow that.

Core logic

1, Try to download from the links passed to --urls first
2. If it doesn't download the package, check if PyPI has a URL for that package
3. If there is a URL, use it to try to download from there

Testing
I've covered all the exit points of main.

@click_log.simple_verbosity_option(_LOGGER)
def main(
url: typing.List[str],
url: typing.Tuple[str],
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: If we're using python 3.9 onwards, you can just use tuple here, rather than typing.Tuple, btw.

Suggested change
url: typing.Tuple[str],
url: tuple[str],

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, Iterable if you don't much mind what kind of iterable it is? from collections.abc import Iterable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

python-rules still supports 3.8

This changes is because click passes it as a tuple rather than list - it's a bad type hint from the original commit


# We're currently hardcoding PyPI but we should consider allowing other
# repositories
# TODO (tm-jdelapuente): allow downloads from other package repositories
Copy link
Contributor

Choose a reason for hiding this comment

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

Why removing this TODO, if not done? I wonder whether a flag could be added with a default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO we're (to a great extent) satisfying this requirement because we're now handling downloads if a URL is passed.

What we don't support is resolving the wheel to download from multiple options in a single repo. Maybe we can create an issue for that if someone feels strongly about it, otherwise this feels like a wishlist item to me

@tm-jdelapuente tm-jdelapuente requested review from SpangleLabs and removed request for SpangleLabs and goddenrich August 20, 2024 12:32
@tm-jdelapuente tm-jdelapuente merged commit 01767c8 into master Aug 20, 2024
@tm-jdelapuente tm-jdelapuente deleted the allow-non-PyPI-downloads branch August 20, 2024 12:45
@tm-jdelapuente tm-jdelapuente mentioned this pull request Aug 20, 2024
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.

3 participants