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.

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

chore(Makefile): Give the help information of Makefile to make it mor… #65

Merged
merged 5 commits into from
Mar 14, 2023

Conversation

cubxxw
Copy link
Contributor

@cubxxw cubxxw commented Mar 3, 2023

Signed-off-by: Xinwei Xiong 3293172751nss@gmail.com
Give the help information of Makefile to make it more readable, give some necessary lab

❯ make help
Usage:

  make [target]


Usage: 

  build         Build the project for horizon
  format        Gofmt (reformat) package sources (exclude vendor dir if existed).
  swagger       Build the swagger
  swagger-run   Run the swagger
  job           Build the job
  core          Build the core
  clean         Clean the project and remove the docker images
  lint          Run the linter
  ut            Run the unit tests
  format        Run go fmt against code.
  help          Display help information

mouuii and others added 2 commits March 4, 2023 02:15
chore(Makefile): Give the help information of Makefile to make it more readable

Signed-off-by: Xinwei Xiong <3293172751nss@gmail.com>
Signed-off-by: Xinwei Xiong <3293172751nss@gmail.com>
# Set the default target to build all
.DEFAULT_GOAL := all

# Declare all targets as phony targets
Copy link
Contributor

Choose a reason for hiding this comment

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

Is default help better?

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 think the default help information is better

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

Choose a reason for hiding this comment

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

why .PHONY is behind command

Makefile Outdated
ut:
sh .unit-test.sh

## format: Run go fmt against code.
Copy link
Contributor

Choose a reason for hiding this comment

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

format -> fmt ?

Makefile Outdated
.PHONY: swagger
## format: Gofmt (reformat) package sources (exclude vendor dir if existed).
.PHONY: format
format: tools.verify.golines tools.verify.goimports
Copy link
Contributor

Choose a reason for hiding this comment

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

no target tools.verify.golines

Makefile Outdated
.PHONY: swagger
## format: Gofmt (reformat) package sources (exclude vendor dir if existed).
.PHONY: format
format: tools.verify.golines tools.verify.goimports
Copy link
Contributor

@mouuii mouuii Mar 4, 2023

Choose a reason for hiding this comment

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

goimport include gofmt,so we use just goimport is ok.

SRC = $(shell find . -type f -name '*.go' -not -path "./vendor/*")

imports:
	@goimports -l -w $(SRC)

Also you can detect files which in git cache stage like below.

for file in $(git diff --cached --name-only --diff-filter ACM | grep "\.go" | grep -v -e "third_party" -e "Godeps"); do
  # Check for files that fail gofmt.
  diff="$(git show ":${file}" | gofmt -s -d)"
  if [[ -n "$diff" ]]; then
    files_need_gofmt="${files_need_gofmt} ${file}"
  fi
done

@cubxxw
Copy link
Contributor Author

cubxxw commented Mar 4, 2023

Please retain the branch and I will resolve these issues before the branch life cycle ends

Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Outdated

.PHONY: swagger
## format: Gofmt (reformat) package sources (exclude vendor dir if existed).
.PHONY: format
Copy link
Collaborator

Choose a reason for hiding this comment

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

it is good to put .PHONY statement together

Makefile Outdated
swagger:
ifeq ($(shell uname -m),arm64)
docker build -t horizon-swagger -f build/swagger/Dockerfile . --platform linux/arm64
else
docker build -t horizon-swagger -f build/swagger/Dockerfile .
endif

.PHONY: swagger-run
## swagger-run: Run the swagger
Copy link
Collaborator

Choose a reason for hiding this comment

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

Run a swagger server locally

Copy link
Collaborator

@kilosonc kilosonc left a comment

Choose a reason for hiding this comment

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

LGTM

@echo "Comments:"
@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
Collaborator

Choose a reason for hiding this comment

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

empty line

# Debugging info
*.debug
*.exe
*.test
Copy link
Collaborator

Choose a reason for hiding this comment

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

empty line

@mouuii
Copy link
Contributor

mouuii commented Mar 9, 2023

LGTM

@cubxxw
Copy link
Contributor Author

cubxxw commented Mar 23, 2023

I need to refer to this issue #100

@cubxxw cubxxw deleted the feature/set-host branch March 23, 2023 15:22
@cubxxw cubxxw restored the feature/set-host branch March 23, 2023 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants