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

Assume an implied caret when a semver doesn't have an explicit constraint #631

Merged
merged 5 commits into from May 27, 2017

Conversation

Projects
None yet
3 participants
@carolynvs
Copy link
Collaborator

carolynvs commented May 23, 2017

Fixes #225

When reading in a manifest, and no constraint has been applied to a version, assume the caret ^ constraint by default. Now a user must add the = prefix to restrict a version to exactly the version specified and maintain the previous behavior.

I decided to accept implied carets but never write out implied carets, i.e. be liberal in what we accept and strict in what we output. So when init generates a manifest, it continues to output carets as dep does today. I prefer this over outputting bare versions because it is more clear to users; it should cut down on issues where someone is surprised that the version wasn't "pinned". I left comments in as to why we aren't using ImpliedCaretString.

Existing tests which relied upon a bare version being interpreted with an = constraint have been updated with explicit = prefixes.

@googlebot googlebot added the cla: yes label May 23, 2017

@carolynvs carolynvs force-pushed the carolynvs:implied-caret branch from f4202e7 to 6e336c1 May 23, 2017

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented May 24, 2017

I decided to accept implied carets but never write out implied carets

I've thought about various ways of doing this, because I am a bit worried that this whole thing will backfire on us. And I've considered something like this.

I think it might work well for people who understand the system, but it would be substantially worse for people who don't - and newbies are the ones we need to optimize for, here. Implicit caret is going to be surprising enough, but if the system writes it one way (with caret), then users choose to write a constraint without a caret then get the same behavior, that'll just be tremendously confusing, because the user is likely to understand their choice relative to the way the system does it.

Especially given the confusion people often have with the unary compatibility operators (^ and ~), I think the real goal here is to truly hide them from people. We just want to acclimate them to the idea that simply putting in a version number is saying "this, or anything newer up to the next major version."

@@ -486,11 +486,11 @@ var basicFixtures = map[string]basicFixture{
},
"simple dependency tree": {
ds: []depspec{
mkDepspec("root 0.0.0", "a 1.0.0", "b 1.0.0"),

This comment has been minimized.

@sdboyer

sdboyer May 24, 2017

Member

omg this is awful and never even occurred to me i can't believe you did all this i hope you wrote a script you're amazing

This comment has been minimized.

@carolynvs

carolynvs May 24, 2017

Collaborator

No worries, it wasn't hard to add, or to remove!

@@ -52,7 +52,7 @@ type Constraint interface {
// If the input string cannot be made into a valid semver Constraint, an error
// is returned.
func NewSemverConstraint(body string) (Constraint, error) {

This comment has been minimized.

@sdboyer

sdboyer May 24, 2017

Member

I think we actually might want to mirror the change in Masterminds/semver instead, and introduce a func NewSemverConstraintIC(). That way we still give the implementing tool the choice as to whether they're using implicit carets or not; gps can remain agnostic.

This comment has been minimized.

@carolynvs

carolynvs May 24, 2017

Collaborator

If I go that route, I can undo the changes to gps testdata, and have gps test without implied carets. So that's a win allaround! ❤️

@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented May 24, 2017

I typed up a lively response about user expectations, reading documentation, and 🥕s, and then realized that the primary goal may not actually be having the least surprising user experience, but instead maximizing the likelihood that libraries use looser constraints, i.e. "the greater good". If we don't expect people to bother reading the docs, then we can take advantage of this behavior so that more people are using carets whether they realize or not.

So on that front, I'm convinced. 😉

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented May 24, 2017

If we don't expect people to bother reading the docs, then we can take advantage of this behavior so that more people are using carets whether they realize or not.

This is absolutely 💯 the goal. We want the non-knowledgeable user's default choice to be one with less likelihood of harming the ecosystem.

the primary goal may not actually be having the least surprising user experience

And this is definitely why I'm worried about it backfiring 😄 On that front, I'm just glad that we're not the first to try this.

@carolynvs carolynvs force-pushed the carolynvs:implied-caret branch from 6e336c1 to 16e350f May 24, 2017

@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented May 24, 2017

Okeedokee!

  • gps supports implied carets in the same way Masterminds/semver does, as an option.
  • dep always uses implied carets, both when reading/writing the manifest and when an unconstrained version is specified in dep ensure
@sdboyer
Copy link
Member

sdboyer left a comment

tiiiiiiny little docs nits 😄

@@ -64,6 +72,24 @@ func NewSemverConstraint(body string) (Constraint, error) {
return semverConstraint{c: c}, nil
}

// NewSemverConstraintIC attempts to construct a semver Constraint object from the
// input string, defaulting to a caret, ^, when no constraint is specified.

This comment has been minimized.

@sdboyer

sdboyer May 25, 2017

Member

nit: s/constraint/operator/

// than =.
//
// In the same way that String() is the inverse of NewConstraint(), this
// method is the inverse of to NewSemverConstraintIC().

This comment has been minimized.

@sdboyer

sdboyer May 25, 2017

Member

nit: "of to"

// than =.
//
// In the same way that String() is the inverse of NewConstraint(), this
// method is the inverse of to NewSemverConstraintIC().

This comment has been minimized.

@sdboyer

sdboyer May 25, 2017

Member

nit: "of to"

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented May 25, 2017

Actually, don't worry about those nits - I'm going to merge this into the running branch, and I'll just fix em there 😄

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented May 25, 2017

(hah those are typos i made in the upstream!)

@sdboyer sdboyer closed this May 25, 2017

@sdboyer sdboyer reopened this May 25, 2017

@sdboyer sdboyer merged commit 16e350f into golang:master May 27, 2017

3 of 4 checks passed

cla/google CLAs are signed, but unable to verify author consent
codeclimate no new or fixed issues
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@carolynvs carolynvs referenced this pull request Jun 7, 2017

Merged

Add -gopath flag to init #497

@carolynvs carolynvs deleted the carolynvs:implied-caret branch Jun 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment