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

style: fix lint errors and update lint timeout #9464

Merged
merged 7 commits into from
Jun 8, 2021

Conversation

augchan42
Copy link
Contributor

@augchan42 augchan42 commented Jun 5, 2021

Description

Closes: #9460

Go linter errors
The go linter fails on many go files, I had to run gofmt -w -s . to recursively gofmt simplify all .go files at first. These changes were not picked up by git (crlf conversion or some such), otherwise there'd be over 1000 changes! It might be good to add this step to contributing.md. This issue may be due to my setup - I use a windows host and a linux guest vm where I do my build. The code is on the windows host filesystem and the cosmos-sdk folder is shared such that the guest vm can do builds against it.

Markdown linter failures - Extra spaces/newlines on .md files:
docs/core/encoding.md:201:163 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
docs/DOC_WRITING_GUIDELINES.md:12 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2]
docs/DOC_WRITING_GUIDELINES.md:13 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 3]

gocritic appendAssign error (this is the only real code change):
x/distribution/legacy/v043/helpers.go:22:18 gocritic appendAssign: append result not assigned to the same slice
make: *** [Makefile:335: lint] Error 1

I also uncommented the 5 minute timeout in the .golangci.yml as I couldn't get make to complete without increasing the timeout. My computer is a 2.6 Ghz Intel I7-10750, so I don't think its because my computer is slow.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • confirmed that this PR does not change production code - no functionality has changed

* gocritic appendAssign fix

* markdown trailing space and blank line fixes

* change golangci-lint timeout to 5min
@github-actions github-actions bot added C:CLI C:x/distribution distribution module related T: CI T:Docs Changes and features related to documentation. labels Jun 5, 2021
@codecov
Copy link

codecov bot commented Jun 6, 2021

Codecov Report

Merging #9464 (ca2e508) into master (a7b0eb1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head ca2e508 differs from pull request most recent head 21f4907. Consider uploading reports for the commit 21f4907 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9464   +/-   ##
=======================================
  Coverage   60.54%   60.55%           
=======================================
  Files         590      590           
  Lines       37287    37288    +1     
=======================================
+ Hits        22577    22578    +1     
  Misses      12763    12763           
  Partials     1947     1947           
Impacted Files Coverage Δ
x/distribution/legacy/v043/helpers.go 100.00% <100.00%> (ø)

Copy link
Contributor

@ryanchristo ryanchristo left a comment

Choose a reason for hiding this comment

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

Thanks for opening a pull request! I added a couple suggestions and comments.

The only changes that I think are necessary is fixing the format in .golangci.yml and maybe updating the changelog entry to be more specific. Let me know if you have any questions.

.golangci.yml Outdated Show resolved Hide resolved
.golangci.yml Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
x/distribution/legacy/v043/helpers.go Show resolved Hide resolved
@augchan42 augchan42 force-pushed the augchan42/9460-makefix branch 2 times, most recently from ff1e44f to aaa6465 Compare June 7, 2021 02:51
augchan42 and others added 2 commits June 7, 2021 10:52
Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>
Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>
@augchan42 augchan42 marked this pull request as ready for review June 7, 2021 02:56
@augchan42 augchan42 changed the title fix: get make working against freshly cloned repo fix: make fails against freshly cloned repo Jun 7, 2021
@ryanchristo ryanchristo changed the title fix: make fails against freshly cloned repo style: fix lint errors and update lint timeout Jun 7, 2021
Copy link
Contributor

@ryanchristo ryanchristo left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes. This looks good to me.

For the pull request type, this is not necessarily a "fix". I think "style" would be better suited for this pull request given that the changes are not fixing a bug but rather fixing lint errors and the functionality of the code remains the same. I'm still not sure if the changelog entry is necessary but I think it's ok to leave it. I have updated the title and the checklist items to use the other template.

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Jun 8, 2021
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. C:CLI C:x/distribution distribution module related T: CI T:Docs Changes and features related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resolve linter errors when running make against clean repo
4 participants