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

bug/python pip argspec #219

Merged
merged 6 commits into from
Jan 23, 2024
Merged

Conversation

blast-hardcheese
Copy link
Collaborator

Why

When installing packages via pip by way of the Packages pane, we interpolate the CLI arg by way of arg + spec.

In looking for why this was the case, it turns out that we impose the same argument specifications on all languages, namely <pkg> <spec>, which gets confusing in situations like flask[async]>=2,<3, where the spec would actually be [async]>=2,<3.

image

Instead, we should defer to language specific parsing semantics where possible, accepting a wider variety of acceptable argspecs, which more closely resemble the args of the underlying commands, to be less confusing.

What changed

  • Language-specific argument parsing overrides
  • Sensible fallback of an empty version constraint to == instead of leaving it unconstrained
  • For pip, maintain the users' version spec all the way through to writing back to requirements.txt, so upm add flask>=2.3.0 doesn't get turned into flask===2.3.3 on save.

Test plan

upm -l python3 --ignored-packages unit_tests --ignored-paths .pythonlibs add 'flask 2'  # Fails, expectedly, since `==2` is not a valid version of flask
upm -l python3 --ignored-packages unit_tests --ignored-paths .pythonlibs add 'flask 2.3.3'
upm -l python3 --ignored-packages unit_tests --ignored-paths .pythonlibs add 'flask'
upm -l python3 --ignored-packages unit_tests --ignored-paths .pythonlibs add 'flask==2.3.3'
upm -l python3 --ignored-packages unit_tests --ignored-paths .pythonlibs add -- 'flask[async]>=2,<3'

these all work as expected in both Poetry and Pip

@blast-hardcheese blast-hardcheese requested a review from a team as a code owner January 20, 2024 01:35
@blast-hardcheese blast-hardcheese requested review from ryantm and removed request for a team January 20, 2024 01:35
@blast-hardcheese blast-hardcheese force-pushed the dstewart/bug/python-pip-argspec branch from c53ae80 to e20e0ce Compare January 22, 2024 20:57
Copy link
Collaborator

@ryantm ryantm left a comment

Choose a reason for hiding this comment

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

Changes look good. I'm a little concerned you didn't have to touch any tests when making them.

@blast-hardcheese blast-hardcheese force-pushed the dstewart/bug/python-pip-argspec branch from e20e0ce to 50a4138 Compare January 22, 2024 22:23
@blast-hardcheese
Copy link
Collaborator Author

blast-hardcheese commented Jan 22, 2024

I'm a little concerned you didn't have to touch any tests when making them.

Turns out we do not have tests around invoking upm add "$arg1" "$arg2" "$arg3". Good call.
I was looking in the wrong place. We do have tests that can be made to handle this case, though we don't have any tests that cover this specifically. I'll add.

@blast-hardcheese blast-hardcheese force-pushed the dstewart/bug/python-pip-argspec branch from d865499 to 72820c7 Compare January 23, 2024 06:39
@blast-hardcheese
Copy link
Collaborator Author

blast-hardcheese commented Jan 23, 2024

image

:woohoo:

@blast-hardcheese blast-hardcheese merged commit f6ebc5f into main Jan 23, 2024
3 checks passed
@blast-hardcheese blast-hardcheese deleted the dstewart/bug/python-pip-argspec branch January 23, 2024 07:46
@blast-hardcheese blast-hardcheese added the bug Something isn't working label Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants