Skip to content
This repository was archived by the owner on Sep 9, 2020. It is now read-only.

Bring README up to date after #489 #991

Merged
merged 6 commits into from
Aug 13, 2017
Merged

Conversation

sdboyer
Copy link
Member

@sdboyer sdboyer commented Aug 11, 2017

What does this do / why do we need it?

README was out of date after merging of #489

What should your reviewer look out for in this PR?

My big question would be whether the diagram is sufficiently helpful to be justified.

Other than that, making sure that I haven't been too cryptic 😄

Fixes #565 😄 😄 😄

README.md Outdated
$ dep ensure -add github.com/some/project github.com/other/project/[email protected]
```

`dep ensure -add`'s behavior varies slightly depending on whether there are already rules in `Gopkg.toml` for the named project(s), as well as whether you already import packages from the named project(s). See `dep ensure -examples` for more sample combinations.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused about how the -add command works. I just tried it out and it didn't append to my Gopkg.toml like the examples said it would.

I have more thoughts but wanted to clear that up before babbling on. 🤓

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just found #989 (comment). Since updating the manifest isn't implemented yet, the -examples should be updated remove the inaccurate help text.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

i'll do that as an atomic commit so it's easy to revert later

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, that was easier than i thought - #994. maybe just leave this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HOT! 🎉

README.md Outdated
2. Add a version constraint on the project to `Gopkg.toml` (Optional, but recommended)
3. Run `dep ensure`

`dep ensure -add` provides some CLI sugar to ease this process:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since ensure is in a half implemented state, we should say exactly what the "sugar" is and it replaces all of steps 1-3, e.g. Alternatively you can run dep ensure -add to add the project to Gopkg.lock and your vendor directory, then you can immediately start using the project in your code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only core behavior missing from ensure's implementation is hoisting a constraint up out of the lock. maybe i should just blow some time on getting that last bit of ensure done tonight 🤔

even then, though, that's the problem - dep ensure -add doesn't actually replace step 1. it's just a stopgap for it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that -add is a fully operational battlestar, my comments are all out-of-date. 😀 Thanks for pushing that through!!!

Since this is the quickstart/readme, how about telling people just the best practice:

  1. Run dep ensure -add github.com/foo/bar. This adds a constraint for the latest version to your Gopkg.toml, then updates the lock and vendor directory.
  2. Import and use the package in your code. ✨

Sparkles optional, but highly recommended.

Copy link
Collaborator

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blarg, sorry I managed to put each comment in it's own review. Happy Friday! 🎉

README.md Outdated
$ dep ensure
```
1. `import` a package from the project in one of your `*.go` source files
2. Add a version constraint on the project to `Gopkg.toml` (Optional, but recommended)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe reword that so that it's more clear what they should add? e.g. Add a [[constraint]] entry...

Revert this once we get in and implement that.
@sdboyer sdboyer requested a review from ibrasho as a code owner August 11, 2017 22:45
@sdboyer
Copy link
Member Author

sdboyer commented Aug 12, 2017

@carolynvs are you good with this now, then?

@carolynvs
Copy link
Collaborator

carolynvs commented Aug 12, 2017

Doh! I need to learn to not reply to comments that have been hidden due to new commits...

I was hoping we could tweak the recommendation for adding a dependency:

Since this is the quickstart/readme, how about telling people only the best practice:

  1. Run dep ensure -add github.com/foo/bar. This adds a constraint for the latest version to your Gopkg.toml, then updates the lock and vendor directory.
  2. Import and use the package in your code. ✨

Sparkles optional, but highly recommended.

It's not a huge deal, and we can merge without it, but I think it will help avoid confusion for new users.

Copy link
Collaborator

@ibrasho ibrasho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sdboyer
Copy link
Member Author

sdboyer commented Aug 12, 2017

ahh ok - yeah, that's fair, i had a similar thought while writing that. yeah, can tweak.

@sdboyer
Copy link
Member Author

sdboyer commented Aug 12, 2017

@carolynvs how dat?

Copy link
Collaborator

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@carolynvs carolynvs merged commit 7bc57a4 into golang:master Aug 13, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In which we beseech a gopher luminary to lend us her brilliance
4 participants