Skip to content

version: fix normalize_version() according to PEP440#344

Merged
abn merged 2 commits into
python-poetry:masterfrom
radoering:fix-normalize-version
May 10, 2022
Merged

version: fix normalize_version() according to PEP440#344
abn merged 2 commits into
python-poetry:masterfrom
radoering:fix-normalize-version

Conversation

@radoering
Copy link
Copy Markdown
Member

Resolves: python-poetry/poetry#5534

  • Added tests for changed code.
  • Updated documentation for changed code.

normalize_version() seems quite buggy supposing that it should do normalizations according to PEP 440. This PR fixes this method and adds tests derived from PEP 440 examples.

Since I wasn't sure if the to_string(short=False) method has some other usage (seems to be more semver than PEP440), I introduced a new property norm_string.

@radoering radoering force-pushed the fix-normalize-version branch from de218b4 to 8382da1 Compare May 4, 2022 18:32
@abn
Copy link
Copy Markdown
Member

abn commented May 4, 2022

My 2c, I would say we return PEP440 compliant string when short is true rather than introducing another property. I'd like to avoid adding more cognitive load here and standardise one way.

@radoering
Copy link
Copy Markdown
Member Author

That was my first thought, too. But it seemed a bit confusing later.

It would be a bit weird especially for ReleaseTag because there is a shorten() and an expand() method. At the moment each long name has its short name (rev <-> r, post <-> -). But for PEP440 normalization rev as well as post are normalized to post. So if you call shorten() and expand() on rev you don't get rev back if we make the proposed change. That may be a bit confusing and even increase the cognitive load.

I'm not sure whether the two different variants (short and long) are useful at all. I tend to say that to_string() should always return a normalized version string (no short parameter). If that would be too much a breaking change, I'd say it may be better to introduce norm_string and maybe deprecate to_string().

Further, I'm not happy with short being a bad parameter name for normalized.

Comment thread src/poetry/core/version/pep440/version.py Outdated
Comment thread tests/utils/test_helpers.py
@radoering radoering force-pushed the fix-normalize-version branch from 8382da1 to 9cc246d Compare May 6, 2022 14:48
@abn abn mentioned this pull request May 8, 2022
Copy link
Copy Markdown
Member

@abn abn left a comment

Choose a reason for hiding this comment

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

Very minor comment. Otherwise lgtm.

Comment thread src/poetry/core/version/pep440/segments.py Outdated
@radoering radoering force-pushed the fix-normalize-version branch from 9cc246d to 7d70b01 Compare May 10, 2022 10:34
@sonarqubecloud
Copy link
Copy Markdown

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 1 Code Smell

No Coverage information No Coverage information
1.6% 1.6% Duplication

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.

Build artifacts omit period from .dev segment (1.2.0b1)

2 participants