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

Fix clang-format to specific version #7350

Merged
merged 15 commits into from
Oct 27, 2020

Conversation

akhilkumarpilli
Copy link
Contributor

Description

  • Fixed clang-format version to 6.0 to avoid version inconsistencies

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

Makefile Outdated Show resolved Hide resolved
@akhilkumarpilli akhilkumarpilli marked this pull request as ready for review September 22, 2020 11:17
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

love that this is being added to download, but two things:

  1. Don't forget to update the contributing.md with how users can use/install this.
  2. I propose this is done through a docker container as the main or secondary option. Many people will not want to download yet another tool to work with the sdk. Packaging them all up into a single docker image makes this 100x simpler

@alessio
Copy link
Contributor

alessio commented Sep 22, 2020

On @marbar3778's 1 - yes please! We need docs. On 2., yes please! Let's avoid handling all possible cases. Just stick to a platform (a specific Linux distro in a Docker container) and instruct people how to use that. @marbar3778 and I can help with that.

@tac0turtle
Copy link
Member

I'd suggest just using:

proto-format:
	@echo "Formatting Protobuf files"
	docker run -v $(shell pwd):/workspace --workdir /workspace tendermintdev/docker-build-proto find ./ -not -path "./third_party/*" -name *.proto -exec clang-format -i {} \;
.PHONY: proto-format

easier then having install scripts

@alessio
Copy link
Contributor

alessio commented Sep 25, 2020

easier then having install scripts

👍 Easier and machine-independent! 👍

@amaury1093
Copy link
Contributor

should we close this PR and track #7332 to use docker?

@tac0turtle
Copy link
Member

should we close this PR and track #7332 to use docker?

someone could push to this branch with only this addition:

proto-format:
	@echo "Formatting Protobuf files"
	docker run -v $(shell pwd):/workspace --workdir /workspace tendermintdev/docker-build-proto find ./ -not -path "./third_party/*" -name *.proto -exec clang-format -i {} \;
.PHONY: proto-format

then call it good.

@amaury1093
Copy link
Contributor

^ ping @akhilkumarpilli

@akhilkumarpilli
Copy link
Contributor Author

proto-format:
	@echo "Formatting Protobuf files"
	docker run -v $(shell pwd):/workspace --workdir /workspace tendermintdev/docker-build-proto find ./ -not -path "./third_party/*" -name *.proto -exec clang-format -i {} \;
.PHONY: proto-format

@marbar3778 This is throwing below error when trying to run. Might be issue with clang-format version installed in it:

Formatting Protobuf files
docker run -v /home/vitwit/go/src/github.com/akhilkumarpilli/cosmos-sdk:/workspace \
--workdir /workspace tendermintdev/docker-build-proto \
find ./ -not -path "./third_party/*" -name *.proto -exec clang-format -i {} \;
Unable to find image 'tendermintdev/docker-build-proto:latest' locally
latest: Pulling from tendermintdev/docker-build-proto
7597eaba0060: Pull complete 
62cc7d673d54: Pull complete 
ffd0e099567e: Pull complete 
4db10ed56389: Pull complete 
f73724348027: Pull complete 
Digest: sha256:81ea2b6c9422a9ec2043f0341b46ee182b2f22632290521f7b09b6c6e220b067
Status: Downloaded newer image for tendermintdev/docker-build-proto:latest
YAML:94:22: error: unknown key 'Delimiter'
  - Delimiter:       pb
                     ^~
Error reading /workspace/./.clang-format: Invalid argument
YAML:94:22: error: unknown key 'Delimiter'
  - Delimiter:       pb
                     ^~
Error reading /workspace/./.clang-format: Invalid argument
YAML:94:22: error: unknown key 'Delimiter'
  - Delimiter:       pb
                     ^~

@tac0turtle
Copy link
Member

what version are you expecting?

we use:

Language: Proto
BasedOnStyle: Google
IndentWidth: 2
ColumnLimit: 0
AlignConsecutiveAssignments: true
AlignConsecutiveDeclarations: true

for our formatting and it doesn't cause issues

@tac0turtle
Copy link
Member

tac0turtle commented Oct 7, 2020

I just commented out:

# RawStringFormats:
#   - Delimiter:       pb
#     Language:        TextProto
#     BasedOnStyle:    google

and it didn't cause any changes other than the normal formatting ones, are you sure you need this formatting rule? Seems like most the rules are unneeded?

@akhilkumarpilli
Copy link
Contributor Author

akhilkumarpilli commented Oct 7, 2020

I just commented out:

# RawStringFormats:
#   - Delimiter:       pb
#     Language:        TextProto
#     BasedOnStyle:    google

and it didn't cause any changes other than the normal formatting ones, are you sure you need this formatting rule? Seems like most the rules are unneeded?

I am just using same config which was pushed previously. And it worked fine when using clang-format-6.0. Not an issue I guess, even if we comment it.

Makefile Show resolved Hide resolved
@tac0turtle tac0turtle added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 27, 2020
@tac0turtle tac0turtle requested review from alessio and anilcse October 27, 2020 09:31
@mergify mergify bot merged commit 8014fc6 into cosmos:master Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants