Default to caret #225

Closed
freeformz opened this Issue Feb 6, 2017 · 8 comments

Comments

Projects
None yet
5 participants
@freeformz
Contributor

freeformz commented Feb 6, 2017

In our discussions we were in favor of adopting Rust's carat default although it's not implemented yet, so I'm opening this issue to track it.

There is a related issue here: Masterminds/semver#41

@freeformz

This comment has been minimized.

Show comment
Hide comment
@freeformz

freeformz Feb 6, 2017

Contributor

We already default detected, on disk packages to ^<tag> during init

Contributor

freeformz commented Feb 6, 2017

We already default detected, on disk packages to ^<tag> during init

@sdboyer sdboyer changed the title from Default to carat to Default to caret Feb 8, 2017

@pksunkara

This comment has been minimized.

Show comment
Hide comment
@pksunkara

pksunkara Mar 10, 2017

Is this about defaulting to caret while generating the manifest file (during dep init) or while generating the lock file (during dep ensure)?

Is this about defaulting to caret while generating the manifest file (during dep init) or while generating the lock file (during dep ensure)?

@sdboyer

This comment has been minimized.

Show comment
Hide comment
@sdboyer

sdboyer Mar 10, 2017

Member

We already default to inserting a caret when possible with dep init. This is about how the manifest is parsed. So, if we encounter this manifest:

{
    "dependencies": {
        "github.com/foo/bar": {
            "version": "1.8.0"
        }
    }
}

Then we would treat the bare 1.8.0 as actually meaning ^1.8.0. If users want to specify 1.8.0 and only 1.8.0, they'd need to put in =1.8.0.

Member

sdboyer commented Mar 10, 2017

We already default to inserting a caret when possible with dep init. This is about how the manifest is parsed. So, if we encounter this manifest:

{
    "dependencies": {
        "github.com/foo/bar": {
            "version": "1.8.0"
        }
    }
}

Then we would treat the bare 1.8.0 as actually meaning ^1.8.0. If users want to specify 1.8.0 and only 1.8.0, they'd need to put in =1.8.0.

@sdboyer sdboyer added this to the Manifest and Lock committable milestone Apr 13, 2017

@sdboyer

This comment has been minimized.

Show comment
Hide comment
@sdboyer

sdboyer Apr 14, 2017

Member

I've now got PRs up for all the obvious/outstanding issues in Masterminds/semver. Hopefully we'll get those merged soon, and will be able to get this in.

Member

sdboyer commented Apr 14, 2017

I've now got PRs up for all the obvious/outstanding issues in Masterminds/semver. Hopefully we'll get those merged soon, and will be able to get this in.

@carolynvs

This comment has been minimized.

Show comment
Hide comment
@carolynvs

carolynvs Apr 15, 2017

Collaborator

I read through Masterminds/semver#41 and I'm still confused on why if dep chooses to default to carets that any change in the semver lib would be required? Why couldn't dep ensure check for bare versions without constraint symbols (~, ^) and add a caret at that time?

Is there another link that I can go Indiana Jones on to get more background?

Collaborator

carolynvs commented Apr 15, 2017

I read through Masterminds/semver#41 and I'm still confused on why if dep chooses to default to carets that any change in the semver lib would be required? Why couldn't dep ensure check for bare versions without constraint symbols (~, ^) and add a caret at that time?

Is there another link that I can go Indiana Jones on to get more background?

@sdboyer

This comment has been minimized.

Show comment
Hide comment
@sdboyer

sdboyer Apr 15, 2017

Member

@carolynvs you might be right - it may be possible to do that. My concern is the breadth of inputs that lib currently allows (possibly wider than we really want to allow for dep - which is another topic of discussion). If we insert unconditionally, then:

  1. It'd insert before invalid semver strings, which would actually just change the name (though this could be mitigated by running semver.NewConstraint() and seeing if it works, then re-running it with a leading caret)
  2. It would not cover cases that involve or/union operators (1.0.0 || 2.4.0, which I guess should become ^1.0.0 || ^2.4.0)
  3. ...this is all I can think of right now.

Hmm, maybe it would work 😄

Member

sdboyer commented Apr 15, 2017

@carolynvs you might be right - it may be possible to do that. My concern is the breadth of inputs that lib currently allows (possibly wider than we really want to allow for dep - which is another topic of discussion). If we insert unconditionally, then:

  1. It'd insert before invalid semver strings, which would actually just change the name (though this could be mitigated by running semver.NewConstraint() and seeing if it works, then re-running it with a leading caret)
  2. It would not cover cases that involve or/union operators (1.0.0 || 2.4.0, which I guess should become ^1.0.0 || ^2.4.0)
  3. ...this is all I can think of right now.

Hmm, maybe it would work 😄

@carolynvs

This comment has been minimized.

Show comment
Hide comment
@carolynvs

carolynvs Apr 16, 2017

Collaborator

It would not cover cases that involve or/union operators (1.0.0 || 2.4.0, which I guess should become ^1.0.0 || ^2.4.0)

Is the union operator supported by dep? I'm not questioning if it should or not, just unsure about the scope of what we plan to support, e.g. only =, ~, ^, etc. We could just try to add the 🥕 and if it isn't a valid semver anymore, leave it alone. 😉

Collaborator

carolynvs commented Apr 16, 2017

It would not cover cases that involve or/union operators (1.0.0 || 2.4.0, which I guess should become ^1.0.0 || ^2.4.0)

Is the union operator supported by dep? I'm not questioning if it should or not, just unsure about the scope of what we plan to support, e.g. only =, ~, ^, etc. We could just try to add the 🥕 and if it isn't a valid semver anymore, leave it alone. 😉

@carolynvs

This comment has been minimized.

Show comment
Hide comment
@carolynvs

carolynvs May 22, 2017

Collaborator

throws herself on the grenade I'll take a stab at this and try to have something to review by tomorrow.

Collaborator

carolynvs commented May 22, 2017

throws herself on the grenade I'll take a stab at this and try to have something to review by tomorrow.

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