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

Improve Linting #12133

Closed
4 tasks
faddat opened this issue Jun 3, 2022 · 5 comments
Closed
4 tasks

Improve Linting #12133

faddat opened this issue Jun 3, 2022 · 5 comments

Comments

@faddat
Copy link
Contributor

faddat commented Jun 3, 2022 •
edited
Loading

Summary of Bug

We can improve linting by:

  • no longer using a diff when linting
  • taking linter errors more seriously? (or was ci hiding them from us? I am confused by what I found)
    • looking for them more carefully?
    • trying not to grandfather them in? (above, not trying to imply anything, merely confused)
  • Rigorously following the advice of linters that we choose to enable for work on the cosmos sdk, and disabling or using //nolint:lintername when we don't want to lint some code or use a certain linter

This will have positive effects downstream, too. By having a standard set of linting processes in the cosmos sdk, downstream projects will have standardized expectations of what linters to obey or ignore when working with the cosmos sdk.

Related pull requests:

Version

all

Steps to Reproduce

use the ci system on the sdk, then compare the result to a local run of golangci-lint

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
This was referenced Jun 3, 2022
Merged
Closed
Closed
mergify bot pushed a commit that referenced this issue Jun 3, 2022
@faddat
chore!: var-naming linter errors (#12135)
b7097c3 
## Description

This PR works towards #12133 and does a fumpt for consistency once sdk.Int is merged.

The suggested merge order is to begin with sdk.Int, merge in the various linter PR's one by one, and then finally merge the changes to CI from #12134 

---

### 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...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [x] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [x] reviewed "Files changed" and left comments if necessary
- [x] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
@alexanderbez
Copy link
Contributor

What do you mean by not doing a diff? Golangci-lint provides errors and if there are any, it blocks the PR until they're resolved. I say we do take them pretty seriously, no?

@faddat faddat mentioned this issue Jun 3, 2022
Merged
19 tasks
@faddat
Copy link
Contributor Author

faddat commented Jun 3, 2022 •
edited
Loading

@alexanderbez

I'm... not sure.

If you have a look through the PR's I made, I was able to solve pretty much countless linting issues.

Like you, I'm sure that I have seen that github action fire.

However, what I put together, defies my eyes re: the github action.

I'd love your thoughts on what happened, because I am not sure.

-- maybe we take them seriously, if CI shows them to us?

I don't know how else to explain my findings today.

@alexanderbez
Copy link
Contributor

If you have a look through the PR's I made, I was able to solve pretty much countless linting issues.

I wonder if that's because linting is only applied against the new code/diff, which I guess is what you mean?

In any case, let's get the actions job up and running and resolve any old code that doesn't conform to the linter. How does that sound?

@faddat
Copy link
Contributor Author

faddat commented Jun 3, 2022

Sounds awesome :)

@faddat faddat mentioned this issue Jun 7, 2022
Open
mergify bot pushed a commit that referenced this issue Jun 30, 2022
@faddat @mergify
chore!: var-naming linter errors (#12135)
c6e4ace 
## Description

This PR works towards #12133 and does a fumpt for consistency once sdk.Int is merged.

The suggested merge order is to begin with sdk.Int, merge in the various linter PR's one by one, and then finally merge the changes to CI from #12134

---

### 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...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [x] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [x] reviewed "Files changed" and left comments if necessary
- [x] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)

(cherry picked from commit b7097c3)

# Conflicts:
#	CHANGELOG.md
#	x/group/keeper/keeper.go
@faddat
Copy link
Contributor Author

faddat commented Jul 22, 2022

Hey, I think this is done :D

@faddat faddat closed this as completed Jul 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

chore: fix suspicious join by using .Dir instead of .Join notional-labs/cosmos-sdk
2 participants
@alexanderbez @faddat