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

Remove gofumpt #1309

Closed
ValarDragon opened this issue Apr 21, 2022 · 1 comment · Fixed by #1318
Closed

Remove gofumpt #1309

ValarDragon opened this issue Apr 21, 2022 · 1 comment · Fixed by #1318

Comments

@ValarDragon
Copy link
Member

ValarDragon commented Apr 21, 2022

Gofumpt doesn't have fix-on-save integration with the golang language server pls as far as I can tell. This makes formatting workflows for gofumpt annoying in many editors and acts as an extra hurdle for newer contributors. I'm locally mitigating this with a git pre-commit hook, but I don't know of any way to automate the installation of that for anyone cloning the project. (Nor do I think a pre-commit hook that has to download a go tool is great to be standardizing on -- will cause hard to debug errors for people without internet access at time of commit)

I recommend we disable gofumpt, and do not enforce any sort of linting in package import order. We can add back gofumpt once theres lint on save.

Perhaps we can add gofumpt'ing the entire code to a script that we execute pre-releases and prior to making new major release branches?

What do others think? cref @alexanderbez @faddat

@faddat
Copy link
Member

faddat commented Apr 21, 2022

I think that makes sense, definitely want to make osmosis as easy to contribute to as possible, and the script you just described would take care of the formatting/consistency issues pretty well.

@mergify mergify bot closed this as completed in #1318 Apr 23, 2022
mergify bot pushed a commit that referenced this issue Apr 23, 2022
…1318)

## What is the purpose of the change

Closes: #1309 , context is in the issue. TL;DR, remove gofumpt from being required in CI due to barriers to entry / lack of ease of use with default IDE setups.

Its kept in make format, and make format is intended to be ran pre-releases

## Brief change log

Remove gofumpt from being required.

## Testing and Verifying

This change is a trivial rework / code cleanup without any test coverage.

## Documentation and Release Note

  - Does this pull request introduce a new feature or user-facing behavior changes? yes
  - Is a relevant changelog entry added to the `Unreleased` section in `CHANGELOG.md`? No -- This is not done, as gofumpt was also added in unreleased code, so this is no net change
  - How is the feature or change documented? N/A
Repository owner moved this from 🔍 Needs Review to ✅ Done in Osmosis Chain Development Apr 23, 2022
mergify bot pushed a commit that referenced this issue Apr 23, 2022
…1318)

## What is the purpose of the change

Closes: #1309 , context is in the issue. TL;DR, remove gofumpt from being required in CI due to barriers to entry / lack of ease of use with default IDE setups.

Its kept in make format, and make format is intended to be ran pre-releases

## Brief change log

Remove gofumpt from being required.

## Testing and Verifying

This change is a trivial rework / code cleanup without any test coverage.

## Documentation and Release Note

  - Does this pull request introduce a new feature or user-facing behavior changes? yes
  - Is a relevant changelog entry added to the `Unreleased` section in `CHANGELOG.md`? No -- This is not done, as gofumpt was also added in unreleased code, so this is no net change
  - How is the feature or change documented? N/A

(cherry picked from commit ed10116)

# Conflicts:
#	.golangci.yml
#	.vscode/settings.json
#	Makefile
#	x/epochs/types/hooks_test.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants