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

RFC: Various changes to CI linting #2855

Closed
wants to merge 10 commits into from
Closed

Conversation

Jacalz
Copy link
Member

@Jacalz Jacalz commented Mar 19, 2022

Description:

I want to open this for various suggested changes regarding the linting we do as part of the CI checks. My suggestion is to replace goimports with gofumpt and to stop using golint. As for the former, I think the stricter formatting is doing a great job (see diff for more info) and for the latter, it has been deprecated for a while now. Most of what golint did (if not even everything) is handled by go vet and staticcheck already.

These changes should also make the static analysis quite a bit faster.

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

Gofumpt has been working out very well for me and I am impressed with what it does.
It is actively maintained and works well as a replacement to goimports.
I also removed golint as it is deprecated. Staticcheck handles our needs now.
@Jacalz Jacalz changed the title RFC: Verious changes to CI linting RFC: Various changes to CI linting Mar 19, 2022
@coveralls
Copy link

coveralls commented Mar 19, 2022

Coverage Status

Coverage increased (+0.01%) to 61.711% when pulling 0541550 on Jacalz:gofumpt into d928f91 on fyne-io:develop.

@Jacalz

This comment was marked as outdated.

- name: Staticcheck
run: staticcheck -go 1.14 ./...
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI, this was removed as it picks it up from the module by default.

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

In general this looks like a good change (except the noted CI failure).

However as it's changing the development style I wonder if it should be discussed with the group? Documentation and example git hooks would all need to be updated to avoid lots of PRs failing on open...

@Jacalz
Copy link
Member Author

Jacalz commented Mar 20, 2022

However as it's changing the development style I wonder if it should be discussed with the group? Documentation and example git hooks would all need to be updated to avoid lots of PRs failing on open...

Indeed. To try an alleviate the problems as much as possible, I have both opened this as RFC and added it to the agenda for the Friday meeting next week. I’m very well aware that this might be disruptive to peoples workflows. I want this to get all the attention that it needs :)

container/split.go Outdated Show resolved Hide resolved
@andydotxyz
Copy link
Member

Indeed. To try an alleviate the problems as much as possible, I have both opened this as RFC and added it to the agenda for the Friday meeting next week. I’m very well aware that this might be disruptive to peoples workflows. I want this to get all the attention that it needs :)

Ah I did not notice that good call!

@Jacalz
Copy link
Member Author

Jacalz commented Mar 22, 2022

Looks like tests are passing now :)

@Jacalz
Copy link
Member Author

Jacalz commented Mar 25, 2022

Doesn't make sense at the moment. Will just drop golint in a different PR.

@Jacalz Jacalz closed this Mar 26, 2022
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.

None yet

3 participants