-
Notifications
You must be signed in to change notification settings - Fork 177
Python 3 is the default nowadays #666
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #666 +/- ##
=======================================
Coverage 53.20% 53.20%
=======================================
Files 13 13
Lines 1109 1109
=======================================
Hits 590 590
Misses 519 519 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I remember being burned by removing this before, but maybe the time is ripe this time around. |
@agronholm Other maintainers claim that some version of Python needs to be defined for stability: However, I fail to see how Although I cannot find anything in the documentation clearly supporting that, I now suspect that |
|
@agronholm From actions/setup-python/README.md:
The curial part of information that I missed from https://github.com/actions/setup-python/blob/main/docs/advanced-usage.md#using-the-python-version-input is:
Should I revert? I still wonder why we use |
|
FWIW I'd suggest a revert here per the twine discussion: it's unfortunately documented (IMO) but |
|
May I ask what even prompted this PR @DimitriPapadopoulos ? Was something broken or were you using |
|
There was a period when they made it an error to not define this input. (v4, I think). But it seems it might not be needed anymore? I'm pretty sure I've been leaving it off recently and it's been fine. Edit: I'm not sure about this, I've been using v4 clearly states either python-version or python-version-file is required. Not seeing a mention otherwise. |
|
Didn't see it missing, so just stuck it in a test PR (henryiii/check-sdist#99). This is what it prints out: So it pretty much does nothing when nothing is set here (though the default Python on the path for the runners may not be terrible). I believe it used to error, now it does not. |
|
Yep, when unset it falls back to the runner's system Python. IMO this is liable to cause confusion in some cases (maybe not here!) since the runner Python is sometimes EOL (meaning failed resolutions for packages that expect non-EOL Pythons). |
|
Nothing was broken, but the documentation of In this case, could the version of Python bundled with |
Are you saying something bad will happen if it stays removed? What's the practical difference between setting it to |
So without Something bad might happen when specifically using older distributions than In short, there might be a risk for |
The practical difference is twofold:
TL;DR: omitting (Or another way of putting it: without |
)" This reverts commit a352b89.
|
Alright, reverted. |
No description provided.