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

Basic implementation & tests of implied carets #52

Merged
merged 1 commit into from
May 2, 2017

Conversation

sdboyer
Copy link
Member

@sdboyer sdboyer commented Apr 14, 2017

I elected to go the route of adding a method to the Constraint interface that allows the caller to decide how the constraint should be converted into a string. A bit of a kluge, but I think really the best of a bad set of options.

Fixes #41.

@sdboyer
Copy link
Member Author

sdboyer commented Apr 14, 2017

ping @mattfarina

@mattfarina
Copy link
Member

@sdboyer Quick question, can't this be stored as an internal value and then merged with the other parts in the String() method? Or, am I missing something? I imagine I'm missing something so I'm asking.

@sdboyer
Copy link
Member Author

sdboyer commented Apr 19, 2017

@mattfarina i started with that approach initially, and yep yep, it works just fine for ranges.

the problem was actually with Version.String(). A Version doesn't know whether it's responding to Version.String() or Constraint.String(), because there's no distinction (Go doesn't make available call-site type information to a called method). This meant that, for an application wanting to consistently use implicit caret behavior, it's not sufficient to rely on Constraint.String() getting it right - ranges will, but versions will not.

Client code wanting to achieve that outcome would have to write extra logic to sniff the underlying type and do the work itself. But that gets tricky (e.g., multiple range handling within unions), so better to just make that function a part of the lib itself.

@sdboyer
Copy link
Member Author

sdboyer commented Apr 27, 2017

bump

@mattfarina mattfarina merged commit 139cc09 into Masterminds:2.x May 2, 2017
@sdboyer
Copy link
Member Author

sdboyer commented May 2, 2017

sweet!

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