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

Feature/flag to disable help text #477

Merged
merged 8 commits into from Apr 30, 2017

Conversation

Projects
None yet
4 participants
@EwanValentine
Copy link
Contributor

EwanValentine commented Apr 28, 2017

No description provided.

@googlebot googlebot added the cla: yes label Apr 28, 2017

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 28, 2017

The flag needs to be on init, not ensure 😄

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 28, 2017

Oh wait, looking at these more...ugh, yeah, because we still have #454 to deal with, those other commands are still writing the manifest. Uggghhhhhh.

Let's put this on hold for a second until we can resolve that. Sorry 😦

@EwanValentine

This comment has been minimized.

Copy link
Contributor

EwanValentine commented Apr 28, 2017

@sdboyer Ensure and remove both use sw.Write so I had to pass a value in for those, unless I make it an optional argument?

@EwanValentine

This comment has been minimized.

Copy link
Contributor

EwanValentine commented Apr 28, 2017

Okay, not a problem :)

@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Apr 28, 2017

I just submitted a fix so that only dep init writes to the manifest (so ensure and remove won't modify it anymore) which I think will help out this PR.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 28, 2017

yep, should be ready to move this ahead

@EwanValentine

This comment has been minimized.

Copy link
Contributor

EwanValentine commented Apr 28, 2017

Ace! Thanks! I'll make those changes shortly

EwanValentine added some commits Apr 28, 2017

@EwanValentine

This comment has been minimized.

Copy link
Contributor

EwanValentine commented Apr 28, 2017

All passing :)

@sdboyer
Copy link
Member

sdboyer left a comment

Just one little nit

@@ -320,9 +320,16 @@ func (sw *SafeWriter) Write(root string, sm gps.SourceManager) error {
return errors.Wrap(err, "failed to marshal manifest to TOML")
}

initOutput := ""

This comment has been minimized.

@sdboyer

sdboyer Apr 28, 2017

Member

nit: more idiomatic to do this as var initOutput string

This comment has been minimized.

@EwanValentine

EwanValentine Apr 29, 2017

Contributor

Good call!

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 28, 2017

We do need to make sure this is covered by tests. Fortunately, there's an easy mechanism, there - for all the tests under cmd/dep/testdata, let's go through and add the -no-examples flag to dep init. That'll have the added benefit of making changes to this helptext (which will inevitably happen) cause a little less churn on the test files.

Once you make the change, you'll need to run go test -update; that'll rewrite the "golden" files. See [the README] on the test harness for more info about all that.

cool?

@EwanValentine

This comment has been minimized.

Copy link
Contributor

EwanValentine commented Apr 29, 2017

I was going to ask how the tests work actually, that's perfect, I'll go through those now

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 29, 2017

@EwanValentine Ah, so what you did covers the newly-added logic, which is great. But I was looking for a bit more; I was hoping you'd go through the existing tests and add -no-examples to their dep init calls, in order to minimize churn on the output files in the future (when this example output inevitably changes).

@EwanValentine

This comment has been minimized.

Copy link
Contributor

EwanValentine commented Apr 29, 2017

Apologies, yeah I see what you mean now, will update shortly

@EwanValentine

This comment has been minimized.

Copy link
Contributor

EwanValentine commented Apr 29, 2017

@sdboyer hopefully this is what you mean

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 30, 2017

@EwanValentine yep, that's exactly what i was going for. Thanks, in we go! 🎉

@sdboyer sdboyer merged commit 1bb83aa into golang:master Apr 30, 2017

4 checks passed

cla/google All necessary CLAs are signed
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

ibrasho pushed a commit to ibrasho-forks/dep that referenced this pull request May 10, 2017

Merge pull request golang#477 from EwanValentine/feature/flag-to-disa…
…ble-help-text

Feature/flag to disable help text
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment