Skip to content

Conversation

@li-jia-nan
Copy link

References

Ignore uppercase and lowercase in clear

@li-jia-nan li-jia-nan requested a review from a team as a code owner September 6, 2024 12:06
@wraithgar
Copy link
Member

What is this supposed to be fixing? What's the use case here? A test would be needed at a minimum.

@li-jia-nan
Copy link
Author

What is this supposed to be fixing? What's the use case here? A test would be needed at a minimum.

Hi, This PR fixes the problem of not supporting V1.2.3 format strings. I added a test case, and I think the capital V should also be cleaned up.

@wraithgar
Copy link
Member

V is not currently a valid prefix in any other part of semver. The reason the v is cleaned is for historical reasons, where it was added sometimes to the version because that's how it is tagged in git.

But because a capital V is not and has not ever been supported, adding this here would then mean it is cleaning a value that is not actually supported by other functions in the library.

For example:

> require('.').parse('v1.0.0')
SemVer {
  options: {},
  loose: false,
  includePrerelease: false,
  raw: 'v1.0.0',
  major: 1,
  minor: 0,
  patch: 0,
  prerelease: [],
  build: [],
  version: '1.0.0'
}
> require('.').parse('V1.0.0')
null

This branch doesn't make V valid in this case, so cleaning it would be improper. I don't think we want to support this. Generally we want semver to reject invalid strings, and not try to "guess" what the user wanted. What's in there now is a legacy setup that would be hard to remove, otherwise we'd try to remove it too.

@wraithgar wraithgar closed this Sep 10, 2024
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.

2 participants