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

fix Makefile style for Go template #5389

Merged
merged 2 commits into from Oct 13, 2018

Conversation

Projects
None yet
3 participants
@toshi0607
Copy link
Contributor

toshi0607 commented Oct 13, 2018

What did you implement:

Closes #XXXXX
no issues

I fixed coding style in Makefiles. build should be treated like clean and deploy in same Makefile.

https://www.gnu.org/software/make/manual/html_node/Phony-Targets.html

How did you implement it:

I added 2 .PHONY:s before build commands.

How can we verify it:

$ cd $GOPATH/src/github.com/[your repo]
$ serverless create -t aws-go-dep -p sample-app
$ cd sample-app
$ make build

Todos:

- [ ] Write tests
- [ ] Write documentation

  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@sebito91
Copy link
Member

sebito91 left a comment

I think this is fine but let's please just combine the .PHONY definitions to a single line.

@toshi0607

This comment has been minimized.

Copy link
Contributor

toshi0607 commented Oct 13, 2018

@sebito91 Thank you for your review! Why do you prefer a single line definition?
It seems that a lot of developers define .PHONY by command and I like the style because it's easy to distinguish the command that is treated as .PHONY from others.
https://github.com/search?l=Makefile&p=1&q=PHONY&type=Code

I think you have the reason to tell that so I'd like to hear it.

@sebito91

This comment has been minimized.

Copy link
Member

sebito91 commented Oct 13, 2018

All stylistic reasons. Technically you are correct but it starts to look a little cluttered as we add more elements to the makefile.

@sebito91
Copy link
Member

sebito91 left a comment

lgtm2

@sebito91 sebito91 merged commit 1ccd144 into serverless:master Oct 13, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.009%) to 91.093%
Details

@sebito91 sebito91 referenced this pull request Oct 13, 2018

Merged

add aws-go-mod #5393

7 of 7 tasks complete

@horike37 horike37 added this to the 1.33.0 milestone Oct 14, 2018

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