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

golangci-lint #358

Closed
wants to merge 27 commits into from
Closed

golangci-lint #358

wants to merge 27 commits into from

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Oct 26, 2022

Golangci-lint github action and golangci lint configuration.

All PR's just submitted will be used in the base of the wasmerless cosmwasm, but are also good for the wasmer version.

@ethanfrey
Copy link
Member

All PR's just submitted will be used in the base of the wasmerless cosmwasm

Did I miss something? If there is a fork, it would be good to know the place, so we can link it for people interested. I got this question from some people wishing to use an interpreter

@faddat
Copy link
Contributor Author

faddat commented Oct 26, 2022

Ah, no fork

Sorry, don't mean to alarm. This is how I start any code task, clean out the repo & automate as much as possible.

In fact, what I'd like to see happen, is that we have mutually compatible implementations:

Contracts are exactly the same.

When we run those contracts in a go environment (ex: wasmd) they run without cgo dependencies

When we eg: cw-sdk, they run rust only (as they already do)

Goal: when we run CosmWasm contracts on a chain written in Go, we do so without using cgo because it makes performance wonky in every case I see it used.

Goal: when we use the cw-sdk, we all light a candle for larry.

@faddat
Copy link
Contributor Author

faddat commented Oct 26, 2022

@webmaster128 Please let me know if you're good merging this PR, I am more than happy to handle its integration into wasmd.

Goal: you change nothing and continue to do your thing in rust.

PS if this doesn't work please let me know.

concerning the question of a cgo-less fork, take this PR and the others that I made today, and this will form the basis of it.

But the goal is actually to make cw much faster in go chains, and change nothing in rust.

Anyhow, all code will be PR'd to this repo :)

@webmaster128
Copy link
Member

Looks like some nice changes in the diff. We already have a Go sanity job in CircleCI and the tools should probably go there instead of introducing GitHub Actions here.

Getting rid of cgo is certainly not a goal any time soon.

I'll try to look into this around mid November.

@faddat
Copy link
Contributor Author

faddat commented Oct 26, 2022

ok man, it is definitely not the biggest hurry. Notes:

  • when I do any code, I always lint like a madman
  • if the circleci stuff did not flag these issues, it isn't tight enough imo
  • I also gofumpt
  • I also check deps
  • I also dependabot

So, those are done now, and basically my plan is I will make a branch, using all of those. After that we.... well, yes, I agree with you, it will be a hard ride to work out the right way to run the wasm in go only. However when that is done, I suspect that go chains will run things much more smoothly.

Some folks have always wondered why I am pretty strict on all of the items above-- basically, from there, I have a base where my editor is much more useful. All warnings have been eliminated, and making modifications is much easier.

@faddat
Copy link
Contributor Author

faddat commented Dec 20, 2024

basically, linting is an important best practice, which helps to prevent bugs.

@webmaster128
Copy link
Member

webmaster128 commented Dec 20, 2024

I just tried to enable stylecheck separately which seems to cause a good amount of the changes in the diff here. But it does not have an auto-fixer, right? Did you do all the code changes manually? Or is there some trick to get a fixer to work?

@faddat
Copy link
Contributor Author

faddat commented Dec 20, 2024

I did it manually, to prevent bugs. It took a long time.

Stylecheck might not be the most important linter.

Some of the other linters, certainly found bugs.

@faddat faddat changed the title Golang ci golangci-lint Dec 20, 2024
@webmaster128
Copy link
Member

I'm happy for those changes to get in, but could you do them one by one using our existing linter setup? There is make lint and make lint-fix available and this is checked in the CI already. No need to copy a linter config from a different project which just creates large diffs. Bumping a min Go version does not belong here, there is not reason to diable goimports and I see no reason for disable-all: true. If we can break the changes down, we can discuss and merge them one by one.

I was looking at stylecheck at first because this is trivial to add to the list and seems to cause most of the changes. But if you think another linter are more important, let us know which one that is.

@faddat
Copy link
Contributor Author

faddat commented Dec 22, 2024

You use disable-all: true to ensure that nothing random gets in there.

I am currently doing testifylint.

Sir, tbh, the linting shows a LOT of issues.

I will make PRs provided that you'll review them and quit canceling contributors.

Do understand: this PR was complete, years ago.

And showed the issues then, too.

@faddat
Copy link
Contributor Author

faddat commented Dec 22, 2024

faddat@Jacobs-MacBook-Pro swappable-vm % golangci-lint run ./... --fix
internal/api/lib_test.go:858:12: go-require: MockInfoBin contains assertions that must only be used in the goroutine running the test function (testifylint)
                                info = MockInfoBin(b, "fred")
                                       ^
internal/api/lib_test.go:861:5: go-require: require must only be used in the goroutine running the test function (testifylint)
                                require.NoError(b, err)
                                ^
internal/api/lib_test.go:862:5: go-require: requireOkResponse contains assertions that must only be used in the goroutine running the test function (testifylint)
                                requireOkResponse(b, res, 0)

for example....

basically there is a lot of test here, that is not test.

And more.

Much more.

I needed testifylint to work on the go implementation of the VM. Please, don't view that as a competitive effort.

The fact is I have seen cosmwasm work out very well as a language, but libwasmvm isn't very good, mainly because of all of the interop stuff.

Would be interesting to jfdi this.

@faddat
Copy link
Contributor Author

faddat commented Dec 23, 2024

@faddat faddat closed this Dec 23, 2024
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