Skip to content

Add gofmt check to travis CI#133

Closed
Fedosin wants to merge 1 commit intogophercloud:masterfrom
Fedosin:travis_gofmt
Closed

Add gofmt check to travis CI#133
Fedosin wants to merge 1 commit intogophercloud:masterfrom
Fedosin:travis_gofmt

Conversation

@Fedosin
Copy link
Copy Markdown
Contributor

@Fedosin Fedosin commented Dec 7, 2020

This change allows to keep the code in a better shape automatically.

This change allows to keep the code in a better shape automatically.
@EmilienM
Copy link
Copy Markdown
Contributor

EmilienM commented Dec 7, 2020

👍

@jtopjian
Copy link
Copy Markdown
Contributor

jtopjian commented Dec 8, 2020

@Fedosin Thanks for submitting this

This is kind of already taken care of here: https://github.com/gophercloud/utils/blob/master/script/format

The above script uses goimports which also does formatting. However, it's lacking support for simplifying code.

Googling "goimports simply" returns some Github discussion about this (I won't link here as I'm too shy to have this PR referenced in those larger ones 🙂 ). It might be best to refactor format so that it calls both goimports and gofmt.

If you wanted to do this, go for it.

The reason the format script might look a little weird is because it is based off of this one from Gophercloud. It accounts for a bunch of edge cases we've run into over the years where Go was trying to format wrong files or incorrectly flagging files because of Golang format bugs (for example, these files once triggered go fmt to report errors even though they were fine. I should check and see if this is still the case).

I recommend setting up go fmt to only scan the files from the find_files function as well as have the ability to skip files, too. It might only happen on a rare occasion, but having Travis blocked because of a Golang format bug isn't fun.

Let me know if you have any questions.

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.

3 participants