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

Ensure version contains an operator when defined #5765

Merged
merged 3 commits into from
Jul 4, 2023

Conversation

sebastien-coavoux
Copy link
Contributor

@sebastien-coavoux sebastien-coavoux commented Jun 30, 2023

Try to fix #5764

Maybe not the best as it simply revert one line to avoid the crash. I've added a test to validate the fix

I believe there might be a better fix by concatenating a "default operator" to _pipfile["version"] before calling the specifier constructor. I am just not sure of the intent of the whole function and which function should add this default operator.
Requirement.parse function ?

You have probably more knowledge on this to see if a default operator make sense. At least, we have a base to fix

@matteius Let me know

@sebastien-coavoux
Copy link
Contributor Author

All right, I kinda changed my though as I realized that get_version is called multiple time and that it was already handling ==X.Y.X "correctly when passed as parameter.

Reusing regex defined in another module to ensure we have something like an operator and defaulting to "==" if not.

@matteius What do you think ?

@@ -92,6 +92,23 @@ def test_install_without_dev(pipenv_instance_private_pypi):

@pytest.mark.basic
@pytest.mark.install
def test_install_with_version_req(pipenv_instance_pypi):
Copy link
Member

Choose a reason for hiding this comment

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

I would recommend naming this test like: test_install_with_version_req_default_operator

Copy link
Contributor Author

@sebastien-coavoux sebastien-coavoux Jul 3, 2023

Choose a reason for hiding this comment

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

Done. Also fixed some failing test. Not sure how to trigger the CI again to check. Test are failing on main branch for me locally, trying to the get working , but right now not 100% sure I fixed them all

@sebastien-coavoux sebastien-coavoux changed the title Do not override specifier if not defined Ensure version contains an operator when defined Jul 3, 2023
@sebastien-coavoux
Copy link
Contributor Author

https://github.com/pypa/pipenv/actions/runs/5445922252/jobs/9906282101 => macOS 3.8 is red. Trying to see how I can fix it

@matteius
Copy link
Member

matteius commented Jul 3, 2023

@sebastien-coavoux Don't worry about that sporadic test failure, its unrelated to your change.

@matteius
Copy link
Member

matteius commented Jul 3, 2023

@sebastien-coavoux Please add a news fragment but I think it looks good -- will add @oz123 for review.

@matteius matteius requested a review from oz123 July 3, 2023 16:47
@sebastien-coavoux
Copy link
Contributor Author

@matteius you mean update Changelog.rst right?

@matteius
Copy link
Member

matteius commented Jul 3, 2023

@matteius you mean update Changelog.rst right?

No, that is generated from news fragments which get deleted from the news file. Go ahead and create a file in news 5765.bugfix.rst and with a brief description of the fix.

@sebastien-coavoux
Copy link
Contributor Author

sebastien-coavoux commented Jul 3, 2023

I ran towncrier create 5765.bugfix -c 'TEXT' to generate the file above, I think that's what you expected

@matteius matteius merged commit 343f020 into pypa:main Jul 4, 2023
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.

Default specifier operator regression from v2023.6.18
2 participants