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

Python full version in pipfile #5387

Merged
merged 4 commits into from
Oct 5, 2022

Conversation

jerempy
Copy link
Contributor

@jerempy jerempy commented Oct 3, 2022

Thank you for contributing to Pipenv!

The issue

#5345

A user requested that when the full version is specified that it gets added to a new pipfile when it is created so that he/she wouldn't need to manually add it themselves.

The fix

Found the spot where the pipefile [requires] section gets created and modified it slightly to save based on if full version was specified or not.

The checklist

  • Associated issue
  • A news fragment in the news/ directory to describe this fix with the extension .bugfix.rst, .feature.rst, .behavior.rst, .doc.rst. .vendor.rst. or .trivial.rst (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

expected output doesn't match current output which is "python_version": "3.10"
Copy link
Contributor

@oz123 oz123 left a comment

Choose a reason for hiding this comment

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

Thank you for your PR.

Please fix the linting issue, and add a news fragment.

This is then good to be merged!

@jerempy
Copy link
Contributor Author

jerempy commented Oct 4, 2022

thanks @oz123 ! we should be good to go now 😎

@oz123
Copy link
Contributor

oz123 commented Oct 4, 2022

Please rename the news snippet to: 5345.behavior.rst

@oz123
Copy link
Contributor

oz123 commented Oct 4, 2022

As I suspected this change leads to breakage of CI.

Successfully set up CPython (3.7.14)

I think that pinning to a minor version is not a good thing. Unless it's additional. What I mean is, we should have both, e.g.:

python_version=3.7
python_full_verison=3.7.14

and news segment
@jerempy jerempy force-pushed the python_full_version-in-pipfile branch from f4472d2 to 5bf9f1b Compare October 5, 2022 14:52
@jerempy
Copy link
Contributor Author

jerempy commented Oct 5, 2022

@oz123 I updated it so now the pipfile will record both the python version and the full version in addition if specified. Also updated tests so should work for whatever python version the test is running. Sorry for the back and forth and hopefully we good to go now 🤞

news/5345.behavior.rst Outdated Show resolved Hide resolved
@oz123
Copy link
Contributor

oz123 commented Oct 5, 2022

OK. Already very promising. Just add some more explanation to the news file, and we are good to go.

@oz123 oz123 requested a review from matteius October 5, 2022 21:21
@matteius
Copy link
Member

matteius commented Oct 5, 2022

What happens if a new minor version is released? Will users have to manually update the python_full_version?
EDIT: Ah -- nevermind -- I see it only populates this if a full version is specified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants