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

fix: validate whether a package is an URL correctly #1356

Merged
merged 5 commits into from
Apr 22, 2024

Conversation

huxuan
Copy link
Member

@huxuan huxuan commented Apr 19, 2024

…ctly

  • I have added a news fragment under changelog.d/ (if the patch affects the end users)

Summary of changes

Fix #1355

Test plan

Tested by running

# command(s) to exercise these changes
nox -s tests

P.S. Mark it as draft as depends on discussion #1354

tests/test_upgrade.py Outdated Show resolved Hide resolved
tests/test_upgrade.py Outdated Show resolved Hide resolved
src/pipx/main.py Outdated Show resolved Hide resolved
src/pipx/main.py Outdated Show resolved Hide resolved
src/pipx/main.py Outdated Show resolved Hide resolved
src/pipx/main.py Outdated Show resolved Hide resolved
src/pipx/main.py Outdated Show resolved Hide resolved
changelog.d/1355.bugfix.md Outdated Show resolved Hide resolved
@huxuan huxuan force-pushed the xuan.hu/validate_package_arg branch from 2cee943 to e2a3dd5 Compare April 19, 2024 23:25
@huxuan huxuan changed the title fix: validate absolute path as packages arg on Windows correctly fix: Validate whether a package is an URL correctly. Apr 19, 2024
@huxuan huxuan changed the title fix: Validate whether a package is an URL correctly. fix: Validate whether a package is an URL correctly Apr 19, 2024
@huxuan huxuan changed the title fix: Validate whether a package is an URL correctly fix: validate whether a package is an URL correctly Apr 19, 2024
@huxuan
Copy link
Member Author

huxuan commented Apr 19, 2024

Hi @chrysle, thanks for your review.

I have a little concern whether we should combine the check for package argument in one place. Currently we need to check it is not a URL, and according to the discussion in #1354, we also need check it is not a absolute path. Any ideas?

@chrysle
Copy link
Contributor

chrysle commented Apr 20, 2024

Hmm, since we only need the path validation in one place, but can reuse the newly introduced function, I'd be inclined to keep it as-is. Or do you mean something else?

@huxuan
Copy link
Member Author

huxuan commented Apr 20, 2024

Hmm, since we only need the path validation in one place, but can reuse the newly introduced function, I'd be inclined to keep it as-is. Or do you mean something else?

I thought to combine all the validation for package and packages argument in one function, but I did not releaize we also need to ensure the spec argument is not a URL but not for an absolute path. So it is fine to keep it as it is now.

In this way, we need another function to fix #1354, something like package_is_abspath. This will be used in the validation for package and packages but not for spec.

Let me know if there is anything wrong.

BTW, if so, then this pull request should be ready for review.

@huxuan huxuan marked this pull request as ready for review April 20, 2024 12:34
@huxuan huxuan force-pushed the xuan.hu/validate_package_arg branch from e2a3dd5 to e024312 Compare April 20, 2024 23:26
@chrysle chrysle merged commit 7e6bc71 into pypa:main Apr 22, 2024
14 checks passed
@chrysle
Copy link
Contributor

chrysle commented Apr 22, 2024

Thanks!

@huxuan huxuan deleted the xuan.hu/validate_package_arg branch April 23, 2024 17:22
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.

pipx upgrade should validate windows absolute path correctly
2 participants