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

feature(makefile): Add help and necessary comments to the makefile to… #1679

Merged
merged 5 commits into from
Mar 28, 2023
Merged

feature(makefile): Add help and necessary comments to the makefile to… #1679

merged 5 commits into from
Mar 28, 2023

Conversation

cubxxw
Copy link
Contributor

@cubxxw cubxxw commented Mar 19, 2023 •
edited
Loading

feature(makefile): Add help and necessary comments to the makefile to make it more readable
style(pkg/common): improve code readability and maintainability

  • Improve variable names for readability

Summary

I want to add help and necessary comments to the makefile to make it more readable.

Output

❯ make help

Usage:

  make [target]

Targets:

  bUild                   Build the program
  all                     Run clean, verify, test, and build operations
  clean                   Clean the workspace
  format                  Format the code
  generate                Generate mocks
  lint                    Check the code
  test                    Run unit and acceptance tests
  unit                    Run unit tests
  acceptance              Run acceptance tests
  acceptance-all          Run all acceptance tests
  prepare-for-pr          Run clean, verify, and test operations and check for uncommitted changes
  verify                  Run format and lint checks
  verify-format           Verify the format
  benchmark               Run benchmark tests
  package                 Package the program
  install                 Install the program to the system
  install-mockgen         Used only by apt-get install when installing ubuntu ppa
  install-goimports       Install goimports dependency
  install-golangci-lint   Install golangci-lint dependency
  mod-tidy                Tidy Go modules
  tidy                    Tidy modules and format the code
  out                     Make a directory for output

Before

After

Documentation

  • Should this change be documented?
    • Yes, see #___
    • No

Related

Resolves #___

@cubxxw
feature(makefile): Add help and necessary comments to the makefile to…
b7f1563 
… make it more readable

Signed-off-by: Xinwei Xiong(cubxxw) <3293172751nss@gmail.com>
@cubxxw cubxxw requested review from a team as code owners March 19, 2023 11:38
@github-actions github-actions bot added this to the 0.29.0 milestone Mar 19, 2023
@github-actions github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Mar 20, 2023
dfreilich
dfreilich requested changes Mar 21, 2023
View reviewed changes
Copy link
Member

@dfreilich dfreilich left a comment

Choose a reason for hiding this comment

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

Thank you for the help!

Makefile Outdated
# .DEFAULT_GOAL := build
# .PHONY: clean build format imports lint test unit acceptance prepare-for-pr verify verify-format benchmark

## bUild: Build the program
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## bUild: Build the program
## build: Build the program

Makefile Outdated
Comment on lines 53 to 54
# this target must be listed first in order for it to be a default target,
# so that ubuntu_ppa's may be constructed using default build tools.
Copy link
Member

Choose a reason for hiding this comment

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

IMO I think we should leave this. Notes with previous context can be useful

Makefile Outdated
Comment on lines 99 to 105
# append coverage arguments
ifeq ($(TEST_COVERAGE), 1)
unit: GOTESTFLAGS:=$(GOTESTFLAGS) -coverprofile=./out/tests/coverage-unit.txt -covermode=atomic
endif
ifeq ($(NO_DOCKER),)
unit: GOTESTFLAGS:=$(GOTESTFLAGS) --tags=example
endif
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the removal of this is causing our tests to fail, because it's not correctly generating the unit test coverage files. I think we should put them back in

Makefile Outdated
Comment on lines 147 to 150
# NOTE: You can analyze the results, using go tool pprof. For instance, you can start a server to see a graph of the cpu usage by running
# go tool pprof -http=":8082" out/bench_cpu.out. Alternatively, you can run go tool pprof, and in the ensuing cli, run
# commands like top10 or web to dig down into the cpu and memory usage
# For more, see https://blog.golang.org/pprof
Copy link
Member

Choose a reason for hiding this comment

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

As above, I think that this can be a useful note to leave in the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'm sorry I shouldn't have dropped it. Thanks for reminding me

Makefile Outdated
@echo "Targets:"
@echo ""
@awk -F ':|##' '/^[^\.%\t][^\t]*:.*##/{printf " \033[36m%-20s\033[0m %s\n", $$1, $$NF}' $(MAKEFILE_LIST) | sort
@sed -n 's/^##//p' ${MAKEFILE_LIST} | column -t -s ':' | sed -e 's/^/ /'
Copy link
Member

Choose a reason for hiding this comment

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

nit: Add extra line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are these empty lines necessary? Because it makes it look a lot clearer

Makefile Show resolved Hide resolved
@cubxxw
fix(makefile): Improve the contents of the file file
9f84736 
Signed-off-by: Xinwei Xiong(cubxxw) <3293172751nss@gmail.com>
@github-actions github-actions bot added type/enhancement Issue that requests a new feature or improvement. and removed type/enhancement Issue that requests a new feature or improvement. labels Mar 22, 2023
@cubxxw
style(pkg/common): improve code readability and maintainability
687ec01 
Signed-off-by: Xinwei Xiong(cubxxw) <3293172751nss@gmail.com>
@cubxxw cubxxw requested a review from dfreilich March 22, 2023 11:28
dfreilich
dfreilich approved these changes Mar 26, 2023
View reviewed changes
Copy link
Member

@dfreilich dfreilich left a comment

Choose a reason for hiding this comment

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

Thank you for the help!

@dfreilich dfreilich modified the milestones: 0.29.0, 0.30.0 Mar 26, 2023
@jkutner jkutner enabled auto-merge March 28, 2023 21:01
@jkutner jkutner merged commit d781d82 into buildpacks:main Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dfreilich dfreilich dfreilich approved these changes

Assignees
No one assigned
Labels
type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Milestone
0.30.0
Development

Successfully merging this pull request may close these issues.
3 participants
@cubxxw @dfreilich @jkutner