Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Build setup and Cosmetics #58

Merged
merged 1 commit into from
Nov 5, 2017
Merged

Build setup and Cosmetics #58

merged 1 commit into from
Nov 5, 2017

Conversation

jawher
Copy link
Contributor

@jawher jawher commented Nov 5, 2017 •
edited
Loading

This is not much, but here goes:

This PR proposes the following (1 commit per proposal):

  • Adding a .gitignore file to ignore the ng binary
  • Adding a Makefile with the following targets:
    • test: to ... run the tests
    • build: to generate the ng binary
    • check: fails if any of go vet, golint, gofmt or ineffasign detects any error. This one will be called by travis so that any future PR is checked for these.
  • Reformatting the code according to gofmt -s so that the travis build passes
  • Small tweaks to make ineffassign happy

@jawher jawher force-pushed the add-makefile branch 3 times, most recently from 6ae6dc5 to 1a282e7 Compare November 5, 2017 11:42
crawshaw
crawshaw requested changes Nov 5, 2017
View reviewed changes
Copy link
Member

@crawshaw crawshaw left a comment

Choose a reason for hiding this comment

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

Thanks for sending a pull request!

I think it's too soon for a Makefile. Most of what's in there I don't want, and we are a ways from the pieces I do want in there (building .deb, snaps, rpms, etc). There is one really nice thing in there, which is gofmt, please see the comment below about the precommit hook.

.gitignore Outdated
@@ -0,0 +1 @@
ng
Copy link
Member

Choose a reason for hiding this comment

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

I use go install neugram.io/ng for building the binary, so I'm never left with one floating around the tree. I'd rather not encourage a build process that does.

Makefile Outdated
vet:
go vet

fmtcheck:
Copy link
Member

Choose a reason for hiding this comment

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

This would be better as a precommit hook. Here's one you can use as a model:

https://golang.org/misc/git/pre-commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having a pre-commit hook will not guarantee that the code is well formatted: Some tools by-pass the hooks, committing from GoLand for example.

Having such a check performed in Travis for every pull request will, imho, be a surefire guarantee that the proposed changes are well formatted.

Makefile Outdated
golint -set_exit_status .

vet:
go vet
Copy link
Member

Choose a reason for hiding this comment

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

I recommend developing at tip until Go 1.10 is out, for a number of plugin bug fixes that are in the tree. A side effect of that is that go vet is now run every time go test is run, so there's no need for extra steps any more.

Makefile Outdated
ineffassign .

setup:
go get github.com/Masterminds/glide
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather avoid these external dependencies for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was a mistake for my part, sorry: glide is not needed (This is a leftover from copy/pasting).

If I get rid of ineffassign, this will just leave go-getting golint. Still unacceptable ? FYI this will only be called in travis and users of ng won't be impacted by this.

eval/shell/job.go Outdated
@@ -313,13 +313,13 @@ func (j *Job) setupSimpleCmd(cmd *expr.ShellSimpleCmd, sio stdio) (*proc, error)
}
switch argv[0] {
case "cd":
dir := ""
var dir string
Copy link
Member

Choose a reason for hiding this comment

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

What is generating these changes? Is it ineffassign? If so, sorry, I disagree with the tool. These two statements are equivalent, and an automated tool shouldn't be choosing one over the other when there's no reason for it.

Copy link

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can get rid of ineffassign.

Copy link

Choose a reason for hiding this comment

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

Aside from that one class of false positive (variables assigned the zero value) everything else ineffassign complains about is a valid bug. If ineffassign isn't run via Travis, it should be run by hand frequently.

@jawher
Copy link
Contributor Author

jawher commented Nov 5, 2017

@crawshaw awaiting your feedback on whether you want to keep the go lint and gofmt checks (in which case I'll update the PR) or not (in which case I'll close it 😄 )

@jawher jawher mentioned this pull request Nov 5, 2017
Closed
@crawshaw
Copy link
Member

crawshaw commented Nov 5, 2017

No golint please. (For now, I need to spend more time getting to know it. The last integrated version I used was too noisy, but it may be a configuration detail.)

A gofmt check in travis is fine, though could we do it without the makefile? Something like: https://gist.github.com/ngdinhtoan/8c43fd92c379c4760735ec7186f215cf

@jawher
Copy link
Contributor Author

jawher commented Nov 5, 2017

@crawshaw Done !

@sbinet
Copy link
Collaborator

sbinet commented Nov 5, 2017

FYI, I have been using this pattern a lot in my projects:
https://github.com/go-hep/hep/blob/master/hep_test.go

A go test Test that runs gofmt or goimports and checks that everything is as it should.

@crawshaw
Copy link
Member

crawshaw commented Nov 5, 2017

@sbinet that's even better, thanks. @jawher would you mind changing this to a go test like that one? Also I'd like to squash this sequence before merging. You can do that if you like, or I can try out this "squash and merge" button.

@jawher
Add a test to ensure the code is properly formatted
813ac5a 
Also reformat the existing files.
@jawher
Copy link
Contributor Author

jawher commented Nov 5, 2017

@crawshaw @sbinet Your feedback has been taken into account, with one change: The TestGoFmt will only look for gofmt (and not goimports) because:

  • gofmt has a -s flag to detect expressions that can be simplified (redundant type names in slices/structs construction, for k, _ := range xs, etc.)
  • Also gofmt detects the imports ordering

@crawshaw
Copy link
Member

crawshaw commented Nov 5, 2017

Thanks!

@crawshaw crawshaw merged commit 81778b6 into neugram:master Nov 5, 2017
@jawher jawher deleted the add-makefile branch November 5, 2017 16:59
@dmitshur
Copy link
Contributor

dmitshur commented Nov 6, 2017

In case it's helpful (for future consideration), here is an IMO good way to run a gofmt check in Travis:

https://github.com/shurcooL/home/blob/99ba63ab813bd15192b0aae77ef21c9b4332c580/.travis.yml#L14

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

@cznic cznic cznic left review comments

@dgryski dgryski dgryski left review comments

@crawshaw crawshaw crawshaw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jawher @crawshaw @sbinet @dmitshur @cznic @dgryski