feat(code-quality): add golangci-lint to CI and lint project #245
Conversation
There was a problem hiding this comment.
Thank you @darkweak for working on this 🙏
This has been on my mind for a while 👍
Just a couple of thoughts:
-
I believe we should tweak the settings to match our use case though.
For example,unusedresultshould probably be disabled (at least for several functions fromterminal). -
We should also take the opportunity to some of those issues instead of masking them (not checking errors for example) which makes me believe it would be better to add the configuration in this PR, maybe fix the obvious issues (
ioutil->osfor example) and fix the other issues with other PRs.
any thought @fabpot ?
IMHO if that's unused that should be removed 😄
That's the first iteration and was to implement asap the golangci-lint. I plan to add some error handling (for those I just disabled checks). |
I'm not referring to unused but unused results that are not always useful and I personally find having tons of
That is the reason why I mentioned enabling only-new-issues. By doing so we can more quickly merge this PR that would only add configuration and most obvious fix and then we can fix other issues in dedicated PRs |
|
Oh sorry, I misunderstood what you said then, thank you for your explanations. |
|
@tucksaun the PR is ready. |
|
Ping @tucksaun |
There was a problem hiding this comment.
I'm stopping here because reviewing this kind of PR is complicated 😖.
Also, the PR needs rebasing.
Would you reconsider opening a new PR with only the configuration and then creating one PR per topic fixed (one by one)?
That would help a lot IMO as they are a couple of fixes that could be merged quickly then.
# We already make sure your own packages wrap errors properly
- github.com/symfony-cli/*
errcheck:
ignore: github.com/symfony-cli/terminal:^(Ep|P)rint
There was a problem hiding this comment.
missing newline at the end of the file
| # We already make sure your own packages wrap errors properly | ||
| - github.com/symfony-cli/* | ||
| errcheck: | ||
| ignore: github.com/symfony-cli/terminal:^(Ep|P)rint |
There was a problem hiding this comment.
missing newline at the end of the file
| if err := os.Chdir(b.Dir); err != nil { | ||
| return errors.WithStack(err) | ||
| } |
There was a problem hiding this comment.
| if err := os.Chdir(b.Dir); err != nil { | |
| return errors.WithStack(err) | |
| } | |
| if err := errors.WithStack(os.Chdir(b.Dir)); err != nil { | |
| return err | |
| } |
I'm not saying we have to do it, but we can do it when there's only a single return value, this also better preserve stack traces IIRC
| } | ||
| } else { | ||
| terminal.Println("Skipped for this step") | ||
| } | ||
|
|
||
| printBanner("<comment>[WEB]</> Stopping the Local Web Server", b.Debug) | ||
| executeCommand([]string{"symfony", "server:stop"}, b.Debug, true, nil) |
There was a problem hiding this comment.
I would tend to think that @fabpot deliberately skip this one
| } | ||
|
|
||
| printBanner("<comment>[SPA]</> Stopping the Local Web Server", b.Debug) | ||
| if _, err := os.Stat(filepath.Join(b.Dir, "spa")); err == nil { | ||
| executeCommand([]string{"symfony", "server:stop", "--dir", filepath.Join(b.Dir, "spa")}, b.Debug, true, nil) |
| @@ -40,16 +41,16 @@ var localProxyAttachDomainCmd = &console.Command{ | |||
| Action: func(c *console.Context) error { | |||
| projectDir, err := getProjectDir(c.String("dir")) | |||
| if err != nil { | |||
| return err | |||
| return errors.WithStack(err) | |||
There was a problem hiding this comment.
this one should be wrapped in the parent function
| } | ||
|
|
||
| homeDir := util.GetHomeDir() | ||
| config, err := proxy.Load(homeDir) | ||
| if err != nil { | ||
| return err | ||
| return errors.WithStack(err) |
| @@ -68,7 +68,7 @@ var localProxyStartCmd = &console.Command{ | |||
| } | |||
| if err := reexec.Background(varDir); err != nil { | |||
| if _, isExitCoder := err.(console.ExitCoder); isExitCoder { | |||
| return err | |||
| return errors.WithStack(err) | |||
There was a problem hiding this comment.
IIRC console.ExitCoder errors must not be wrapped as we have special handling for them in symfony-cli/console
|
@tucksaun I'll open some PRs to review easier. I'll start with one that add only golangci-lint in the project with the given configuration without noises. The PR should be ready this evening. ✌️ |
Hello, this PR is here to have a better code quality/code style using
golangci-lint. It adds the CI job to ensure every other PR will be as well written as possible.