disable the congrats message #112
Conversation
|
How about making it default behaviour? |
|
That would change the current behavior and potentially break someone who for example greps for the congrats message. |
pkg/commands/root.go
Outdated
| @@ -84,6 +84,7 @@ func (e *Executor) needVersionOption() bool { | |||
|
|
|||
| func initRootFlagSet(fs *pflag.FlagSet, cfg *config.Config, needVersionOption bool) { | |||
| fs.BoolVarP(&cfg.Run.IsVerbose, "verbose", "v", false, wh("verbose output")) | |||
| fs.BoolVarP(&cfg.Run.DisableCongrats, "no-congrats", "s", false, wh("disable congrats output")) | |||
There was a problem hiding this comment.
- maybe
silentinstead ofno-congratsandSilentinstead ofDisableCongrats? It will be more general logic. Also in some places options disables not only congrats message, e.g. in JSON s/disable/Disable
There was a problem hiding this comment.
Sure it makes sense.
pkg/printers/json.go
Outdated
| allIssues := []result.Issue{} | ||
| for i := range issues { | ||
| allIssues = append(allIssues, i) | ||
| } | ||
|
|
||
| if len(allIssues) == 0 && j.disableCongrats { | ||
| return true, nil |
There was a problem hiding this comment.
I know it's ugly and we need to refactor it. Here need to return false: it's a flag that issues were found
There was a problem hiding this comment.
Ah ok I missed the logic a bit here
pkg/printers/json.go
Outdated
| @@ -8,22 +8,30 @@ import ( | |||
| "github.com/golangci/golangci-lint/pkg/result" | |||
| ) | |||
|
|
|||
| type JSON struct{} | |||
| type JSON struct { | |||
| disableCongrats bool | |||
There was a problem hiding this comment.
JSON knows nothing about congrats, silent or more specific for JSON like omitEmpty
There was a problem hiding this comment.
My vote is for silent here as well I think.
|
thank you for the pull request! |
|
I will make the suggested changes asap. |
dahankzter
force-pushed
the
master
branch
2 times, most recently
from
30e525f
to
a7d29b8
Compare
pkg/printers/json.go
Outdated
| allIssues := []result.Issue{} | ||
| for i := range issues { | ||
| allIssues = append(allIssues, i) | ||
| } | ||
|
|
||
| if len(allIssues) == 0 && j.silent { | ||
| return false, nil |
There was a problem hiding this comment.
As I mentioned here, I'm not sure this is actually useful to do for JSON output. It's off by default so not a huge deal as such, but the "silent" flag is pretty generically named and could also change other behaviour in the future, and having it output invalid JSON as a kind of side-effect may surprise people.
It may also be confusing in some cases where users provide custom flags to the editor integration or some such, and the editor isn't prepared to deal with invalid JSON, causing confusing errors.
Personally I think always outputting valid JSON is much better, and would simplify the life of people implementing this in editors (i.e. me 😄).
There was a problem hiding this comment.
I agree about valid JSON, it would be better to not change JSON output at all. And checkstyle too, hide only congrats message for text and tab printers
There was a problem hiding this comment.
Sure for JSON we can just emit {} perhaps or simply let it be as it was. @mmatczuk has a point though to consider.
What about the checkstyle xml? Perhaps it should also produce valid xml?
Tab and text can be silenced then is this the temporary consensus until a major decision is made as to the default behavior?
There was a problem hiding this comment.
Ah I just now saw you mentioned checkstyle as well @jirfag
|
@Carpetsmoker I agree that JSON should not be silenced, my point was that maybe it could be just @jirfag I'm not sure if adding the silent flag is a move in right direction
Please reconsider, thanks. |
There is now an extra switch '-s' to disable the congrats message when there are no issues detected Fixes: golangci#110
|
The Checkstyle and JSON output is back to where it was now. |
|
thank you! |
There is now an extra switch '-s' to disable the congrats message when
there are no issues detected
Fixes: #110