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

ADDED example comment to toml file on init #343 #374

Merged
merged 7 commits into from Apr 26, 2017

Conversation

Projects
None yet
4 participants
@EwanValentine
Copy link
Contributor

EwanValentine commented Apr 13, 2017

This is in reference to #343

To test

  1. Create a blank project using $ dep init ensuring no dependencies are detected within the code. I.e a blank directory.
  2. Check Gopkg.toml for some example text, commented out.
@googlebot

This comment has been minimized.

Copy link
Collaborator

googlebot commented Apr 13, 2017

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@googlebot googlebot added the cla: no label Apr 13, 2017

@EwanValentine

This comment has been minimized.

Copy link
Contributor

EwanValentine commented Apr 13, 2017

I signed it!

@googlebot

This comment has been minimized.

Copy link
Collaborator

googlebot commented Apr 13, 2017

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Apr 13, 2017

@EwanValentine

This comment has been minimized.

Copy link
Contributor

EwanValentine commented Apr 13, 2017

I'm having trouble with some of the tests (I'm a new contributor, sorry!).

@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Apr 13, 2017

@EwanValentine I love me some doc in the morning! ❤️

https://travis-ci.org/golang/dep/jobs/221713388#L186 is a panic in a test I wrote, I'll take a look and see what's the matter there.

The remaining tests are failing because the generated file contains your help text, which the tests weren't expecting. The tests have "golden" files in the testdata directories that we compare against. The test runner can update those files for you with the -update flag, so that you don't have to fix them by hand:

go test . ./cmd/dep -update

After you run that, you should see a couple testdata files modified with the new helptext.

@carolynvs
Copy link
Collaborator

carolynvs left a comment

This looks good! Just a couple small changes and we should be all set.

@@ -349,6 +361,13 @@ func (sw *SafeWriter) Write(root string, sm gps.SourceManager) error {
os.RemoveAll(vendorbak)
}

if len(sw.Payload.Manifest.Dependencies) == 0 {

This comment has been minimized.

@carolynvs

carolynvs Apr 13, 2017

Collaborator

If the goal is to only print out the example help text when the manifest is empty, then you will also need to test the manifest's Ovr, Ignores and Required fields as well. It may be useful to combine those checks into an IsEmpty function on the Manifest struct.

This comment has been minimized.

@carolynvs

carolynvs Apr 13, 2017

Collaborator

I looked into the nil pointer error from the build and it is because the Write function doesn't always have a manifest given to it, so sw.Payload.Manifest was nil.

If you move this check under the sw.Payload.HasManifest() check above (https://github.com/golang/dep/blob/master/txn_writer.go#L254), then it will only append the help text when we have a manifest file to write.

// if no dependencies are found in the project
// during `dep init`
const exampleToml = `
# Example:

This comment has been minimized.

@carolynvs

carolynvs Apr 13, 2017

Collaborator

Can you add the remaining available options, revision, version, and source (e.g. https://github.com/myfork/package)? That way a user will know all the values that they can provide, and what an acceptable value looks like for each one.

@@ -349,6 +361,13 @@ func (sw *SafeWriter) Write(root string, sm gps.SourceManager) error {
os.RemoveAll(vendorbak)
}

if len(sw.Payload.Manifest.Dependencies) == 0 {

This comment has been minimized.

@carolynvs

carolynvs Apr 13, 2017

Collaborator

I looked into the nil pointer error from the build and it is because the Write function doesn't always have a manifest given to it, so sw.Payload.Manifest was nil.

If you move this check under the sw.Payload.HasManifest() check above (https://github.com/golang/dep/blob/master/txn_writer.go#L254), then it will only append the help text when we have a manifest file to write.

@EwanValentine

This comment has been minimized.

Copy link
Contributor

EwanValentine commented Apr 13, 2017

@carolynvs Thanks for your feedback, great shout! I've made those changes and updated the test data, still seems to be a panic regarding a missing lock file. Apologies, still getting to grips with the codebase!

# [[dependencies]]
# branch = "master"
# name = "github.com/vendor/package"
# revision = "abc123"

This comment has been minimized.

@EwanValentine

EwanValentine Apr 13, 2017

Contributor

I'm not actually sure what format the revision is in, I couldn't find an example anywhere, is it a git hash?

This comment has been minimized.

@carolynvs

carolynvs Apr 13, 2017

Collaborator

'abc123' is completely fine. I use that all the time as a dummy git commit hash. 😄

This comment has been minimized.

@carolynvs

carolynvs Apr 13, 2017

Collaborator

Looks like we still need an example for source, # source = "https://github.com/myfork/package.git" will do.

This comment has been minimized.

@sdboyer

sdboyer Apr 16, 2017

Member

@EwanValentine "revision" is sort of intentionally underspecified; its valid values vary depending on the underlying source type. For git and hg, it's a SHA1; for bzr, it's this weird 3-part internal revid; for svn, it is (I guess, we actually don't really support this yet) the global svn revision number.

IMO, it's for completeness and clarity, it's probably worth putting in this information (feel free to wordsmith 😛) right there in the comment block, so that users have it immediately on hand. It's at least worth doing that until we have more proper central documentation out there...

@carolynvs
Copy link
Collaborator

carolynvs left a comment

This is so close! 😄 Just a few small changes and the test should all pass.

@@ -252,6 +266,10 @@ func (sw *SafeWriter) Write(root string, sm gps.SourceManager) error {
defer os.RemoveAll(td)

if sw.Payload.HasManifest() {
if sw.Payload.Manifest.IsEmpty() {
err := modifyWithString(mpath, exampleToml)
return errors.Wrap(err, "failed to generate example text")

This comment has been minimized.

@carolynvs

carolynvs Apr 13, 2017

Collaborator

This is unconditionally returning anytime we are writing the helptext, which means that anything else that should have been written (in this case, the lock file) isn't. That's why the travis build is failing (https://travis-ci.org/golang/dep/jobs/221895965#L190).

Also at this point in the code, the manifest should be written to the temp directory, instead of to the final location. The way the safe write works is that first we write to a temp folder, then we save copies of the original files, and then we move the updated files to the final location. If anything goes wrong, then we put back the original files.

I think something like this will get you past the error:

if sw.Payload.HasManifest() {
	if sw.Payload.Manifest.IsEmpty() {
		err := modifyWithString(filepath.Join(td, ManifestName), exampleToml)
		if err != nil {
			return errors.Wrap(err, "failed to generate example text")
		}
	} else if err := writeFile(filepath.Join(td, ManifestName), sw.Payload.Manifest); err != nil {
		return errors.Wrap(err, "failed to write manifest file to temp dir")
	}
}
# [[dependencies]]
# branch = "master"
# name = "github.com/vendor/package"
# revision = "abc123"

This comment has been minimized.

@carolynvs

carolynvs Apr 13, 2017

Collaborator

Looks like we still need an example for source, # source = "https://github.com/myfork/package.git" will do.

manifest.go Outdated
@@ -139,6 +140,14 @@ func (m *Manifest) toRaw() rawManifest {
return raw
}

// IsEmpty - Checks if payload is empty
func (m *Manifest) IsEmpty() bool {
if m.Ovr == nil && len(m.Ignores) == 0 && len(m.Dependencies) == 0 {

This comment has been minimized.

@carolynvs

carolynvs Apr 13, 2017

Collaborator

Let's add a check for m.Required as well and we'll have hit all the fields in the manifest.

@sdboyer
Copy link
Member

sdboyer left a comment

awesome, thanks for jumping on this! A few notes, to go along with @carolynvs' review.

# [[dependencies]]
# branch = "master"
# name = "github.com/vendor/package"
# revision = "abc123"

This comment has been minimized.

@sdboyer

sdboyer Apr 16, 2017

Member

@EwanValentine "revision" is sort of intentionally underspecified; its valid values vary depending on the underlying source type. For git and hg, it's a SHA1; for bzr, it's this weird 3-part internal revid; for svn, it is (I guess, we actually don't really support this yet) the global svn revision number.

IMO, it's for completeness and clarity, it's probably worth putting in this information (feel free to wordsmith 😛) right there in the comment block, so that users have it immediately on hand. It's at least worth doing that until we have more proper central documentation out there...

fs.go Outdated
@@ -75,6 +75,14 @@ func writeFile(path string, in toml.Marshaler) error {
return err
}

// modifyWithString - Modifies a given file with a new string input.

This comment has been minimized.

@sdboyer

sdboyer Apr 16, 2017

Member

nit: s/modifyWithString - Modifies/modifyWithString modifies/

Just slightly more standard practice for docblocks.

@EwanValentine

This comment has been minimized.

Copy link
Contributor

EwanValentine commented Apr 18, 2017

Hey! Sorry I've been away this weekend, will pick this up again tonight :)

EwanValentine added some commits Apr 18, 2017

@EwanValentine

This comment has been minimized.

Copy link
Contributor

EwanValentine commented Apr 18, 2017

I've made those suggested changes and tested locally, similar story with failing builds though!

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 21, 2017

@EwanValentine ah yeah, we changed the name of the Manifest.Ignores field to Manifest.Ignored, for consistency with Required. If you rebase and refactor a bit, tests should start passing 😄

EwanValentine added some commits Apr 21, 2017

@EwanValentine

This comment has been minimized.

Copy link
Contributor

EwanValentine commented Apr 21, 2017

🎉

@carolynvs
Copy link
Collaborator

carolynvs left a comment

Looks good, just one small change left!

manifest.go Outdated
@@ -140,6 +140,14 @@ func (m *Manifest) toRaw() rawManifest {
return raw
}

// IsEmpty - Checks if payload is empty
func (m *Manifest) IsEmpty() bool {
if m.Ovr == nil && len(m.Ignored) == 0 && len(m.Dependencies) == 0 && len(m.Required) == 0 {

This comment has been minimized.

@carolynvs

carolynvs Apr 21, 2017

Collaborator

This should use len(m.Ovr) == 0 like the other fields.

@carolynvs
Copy link
Collaborator

carolynvs left a comment

💖 🎉 🙇

@EwanValentine

This comment has been minimized.

Copy link
Contributor

EwanValentine commented Apr 21, 2017

Awesome! One of my first ever open-source contributions! Thanks for all your help and patience! <3

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 26, 2017

Ah, sorry it took so long for me to come review and click the merge button! Thanks! 🎉

@sdboyer sdboyer merged commit 640786c into golang:master Apr 26, 2017

3 checks passed

cla/google All necessary CLAs are signed
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#374 from EwanValentine/feature/create-toml-…
…file-examples-on-init

ADDED example comment to toml file on init golang#343
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment