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

Don't normalize or escape versions #469

Merged
merged 3 commits into from
Sep 17, 2022

Conversation

dimbleby
Copy link
Contributor

Per python-poetry/poetry#6476

  • normalize_version() is only ever used as an over-elaborate way of calling Version.to_string()
  • escape_version() is actually counter-productive

I've aggressively removed the functions altogether, so would need to merge python-poetry/poetry#6476 before this one (so that poetry no longer uses them).

It would be possible to leave them around unused for a bit, I favour cleaning such things up.

@dimbleby
Copy link
Contributor Author

dimbleby commented Sep 11, 2022

(don't understand why the downstream tests didn't fail, this MR surely removes things that are still used downstream)

@radoering
Copy link
Member

It would be possible to leave them around unused for a bit, I favour cleaning such things up.

I'd prefer a deprecation warning as in

warnings.warn(
"Calling next_prerelease() on a stable release is deprecated for its"
" ambiguity. Use next_major(), next_minor(), etc. together with"
" first_prerelease()",
DeprecationWarning,
stacklevel=2,
)

When removing these functions the next core release will break poetry 1.2.0 unless we bump the major version of core. We could do this, but I don't think there is consensus among maintainers to release one major version after the other.

@dimbleby
Copy link
Contributor Author

I'm going to not do any deprecating for a bit in the hope that y'all reach a consensus that I like better...

If for some reason you decide that merging this is urgent, feel free to reintroduce deprecated versions of the functions.

@dimbleby
Copy link
Contributor Author

When removing these functions the next core release will break poetry 1.2.0 unless we bump the major version of core

I just noticed that this is not true, because poetry 1.2.0 pins to the exact version 1.1.0 of poetry-core: https://github.com/python-poetry/poetry/blob/85993df4fa7b67dbfe573cce7b2a397846b9c320/pyproject.toml#L47

which in some ways doesn't make a lot of difference - I'm in favour of a major version bump here anyway. But does mean that it would be possible to make breaking changes here and not signal that in the version number if for some reason that was deemed desirable.

@radoering
Copy link
Member

I was just surprised to find a fix version in pyproject.toml. 😅

I agree a major bump would be more appropriate if we remove something although I still think deprecation warnings don't hurt and would allow faster progress...

@dimbleby
Copy link
Contributor Author

dimbleby commented Sep 16, 2022

I still think deprecation warnings don't hurt and would allow faster progress...

maybe so, but I don't expect ever to need this fix myself so I'm content to be stubborn about it!

If anyone does turn up who actually cares about the fix in here - eg epoch users per python-poetry/poetry#6466 - then they are welcome to take this and rework it so that escape_version and normalize_version are only deprecated.

Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

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

FYI: Since it's companion has been merged, I'll probably add the deprecation warning myself later.

@sonarcloud
Copy link

sonarcloud bot commented Sep 17, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@radoering radoering merged commit b2e2836 into python-poetry:main Sep 17, 2022
@dimbleby dimbleby deleted the no-normalize-or-escape-version branch September 17, 2022 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants