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

[Regression] (in 3.0.1) Version::getVersionString will not return "v" prefix if used #24

Closed
addshore opened this issue Feb 22, 2021 · 3 comments

Comments

@addshore
Copy link
Contributor

A change happened in 3.0.1 which means that versions will not be output with a prefixed "v" if they started with one.

This was caused by the following change

8cef51d#diff-9cb52248cb807e881c8c2b95169d98aa095f95ee40639b13af3b9f28cc553122L42

Adding this as a regression as the CHANGELOG doesn't mention it.
I spotted this while looking into deprecated-packages/symplify#2972

@addshore
Copy link
Contributor Author

I'm happy to write a patch (if this is deemed to be a regression).
Also happy to add a CHANGELOG entry if this is to be the new accepted behaviour!

@theseer
Copy link
Member

theseer commented Feb 22, 2021

This indeed is a change in behavior.

But, as hard as it may be, the previous behavior was technically speaking wrongly supporting invalid version strings. Any prefix to the version string is not semver compliant (see https://semver.org/#is-v123-a-semantic-version => NO.) and - theoretically - we shouldn't allow it, even when parsing.

So, rather than the output being now "broken" (as compared to before), the parsing shouldn't have even allowed it to begin with. That way, it would at least have been consistent.

Unfortunately, we do support the v-Prefix since day one for input.

As I see it, we have the following options to address this:

  • Leave it as is and report it in the Changelog as a potential BC break
  • Restore the previous behavior by keeping track of the v and prepend it on output
  • Add an optional parameter to Version::getVersionString() for prefixing
  • Add a new method Version::getOriginalString() (or similar) to get the prefix back in case it was specified

I'm not sure which one I like best. As of right now, I'm leaning to preferring the last mentioned option..

@addshore
Copy link
Contributor Author

Add a new method Version::getOriginalString() (or similar) to get the prefix back in case it was specified

Sounds quite good to me too, and this would suite the usecase in symplify/symplify quite well.

addshore added a commit to addshore/version that referenced this issue Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants