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.

Sign up for GitHub

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

Jump to bottom

x/mod/semver: Lack of information about invalid version #49025

Open
mengzhuo opened this issue Oct 18, 2021 · 5 comments
Open

x/mod/semver: Lack of information about invalid version #49025

mengzhuo opened this issue Oct 18, 2021 · 5 comments
Labels
modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@mengzhuo
Copy link
Contributor

Consider "v0.0.1-pre.failed_f+build"

There is no exported method to know what is wrong with this version string with these two method.

semver.IsValid
semver.Canonical

The only way for now is checking all parts of parser

s := "v0.0.1-pre.failed_f+build"
if !semver.IsValid(s) {
   if semver.MajorMinor(s) == "" {
      return err
   }
   if semver. Prerelease(s) == "" {
      return err
   }
   if semver.Build(s) == "" {
     return err
   }
}

What did you expect to see?

Some exported way return with "invalid prerelease"

What did you see instead?

None.

We do have err field but it's been deleted in golang/mod@ecfafd6.

Proposal

Adding semver.Err(string) and revert golang/mod@ecfafd6.

cc @zchee @jayconrod
@gopherbot gopherbot added this to the Unreleased milestone Oct 18, 2021
@zchee
Copy link
Contributor

zchee commented Oct 18, 2021

@jayconrod What do you think? if you like this, I'll send CL.

@mengzhuo
Copy link
Contributor Author

Hi, @zchee

It's just a primitive proposal, I think we should discus about the name of this "Err" method.

@bcmills bcmills added modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 18, 2021
@bcmills
Copy link
Contributor

bcmills commented Oct 18, 2021

I would rather add an explicit Parse function than an Err method.

Pretty much all of the semver functions end up calling the existing parse function and then reporting some subset of the parsed data, and it would be nice to avoid repeating that work for each part of the version we want to extract. That would also provide a convenient (and harder-to-forget) point at which to report errors.

Perhaps something like:

package semver

type Version struct {
	Major, Minor, Patch, Prerelease, Build string
}

func Parse(s string) (Version, error) {
	…
}

@mengzhuo
Copy link
Contributor Author

@bcmills I second this, it is easier to decode/encode with gob/json/binary, etc.

@mengzhuo
Copy link
Contributor Author

@zchee Are you interest in submit CL to fix issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@mengzhuo @bcmills @zchee @gopherbot