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

delete pointless requirements parsing code in dist_utils.py #71

Merged
merged 2 commits into from
Mar 15, 2023

Conversation

cognifloyd
Copy link
Member

All the magic around parsing requirements was entirely unnecessary for this repo as there are NO reqs.

And please! Doing things on import (like raising an error if pip isn't installed) is not ok.
Do not have side effects on import, even (and especially) in setup code.

This fix is a follow-up for #70, and is blocking StackStorm/st2#5932 because importing pip breaks generating the lockfile in the latest version of pants (which uses pex, which has a vendored copy of pip).

Please people. Do not do things on import! Plus, all the magic around parsing
requirements was entirely unnecessary for this repo as there are NO reqs.
@cognifloyd cognifloyd added the bug Something isn't working label Mar 15, 2023
@cognifloyd cognifloyd requested review from winem, arm4b, nzlosh, rush-skills, amanda11 and a team March 15, 2023 01:52
@cognifloyd cognifloyd self-assigned this Mar 15, 2023
@cognifloyd cognifloyd changed the title delete unused requirements parsing code in dist_utils.py delete pointless requirements parsing code in dist_utils.py Mar 15, 2023
Copy link
Member

@rush-skills rush-skills left a comment

Choose a reason for hiding this comment

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

Those bits do seem very wrong in our setup files. Thanks for the fix.

@cognifloyd cognifloyd merged commit 4a00791 into master Mar 15, 2023
@cognifloyd cognifloyd deleted the clean-setup branch March 15, 2023 17:50
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