Consider replacing staticcheck, gosimple, unused with a faster equivalent megacheck.

[dmitshur]

Currently, GopherCI runs three of @dominikh's tools individually:

There is megacheck, which combines all checks from those 3 tools into one tool, and produces results faster (since it doesn't have to perform type checking and discard results 3 times, only once).

Consider using it.

[bradleyfalzon]

Yep agreed, this is better for users and for efficiency for me, but there's a few things I'd like to consider:

• Users will be able to choose which tools they run (#8), so we'd either need 4 options (gosimple, staticcheck, unused or megacheck)
• GopherCI could be a little more intelligent and run megacheck (this is what I did in https://github.com/alecthomas/gometalinter/commit/eff485ae5abeeaa7b53b6a97e932c9974668bc10)

Either of these two options would then mask the underlying tool as just megacheck, this wouldn't be a problem if the user selected that option explicitly, but if we swapped it out for them, could be confusing as they never asked for megacheck. In the future, I believe megacheck will support structured output (https://github.com/dominikh/go-tools/issues/21) making it simpler to detect which tool was responsible for which issue.

In the meantime, when I resolve #8, I might just add megacheck as an option, potentially having it enabled by default, and once megacheck supports structured output do this transparently for users.

[dmitshur]

What you said makes sense as future direction and enhancements. But until #8 is resolved, couldn't the 3 individual tools simply be replaced by megacheck? Is there a reason that shouldn't be done now?

If this can be done now... Is this good material for a first contribution? I'd consider taking it if so.

[bradleyfalzon]

But until #8 is resolved, couldn't the 3 individual tools simply be replaced by megacheck? Is there a reason that shouldn't be done now?

The only reason I didn't do it earlier is because it's not perfectly clear to the user which tool flagged the issue, other than the problem ID's prefix.

Is this good material for a first contribution?

The entire project has little documentation other than https://github.com/bradleyfalzon/gopherci/blob/master/CONTRIBUTING.md and you might end up sending more PRs to that than the original issue.

What's the reason for wanting it? It's good for speed, is that what you're looking for? Or to close off the issue? Either is a good reason.

[fortytw2]

to chime in here (perhaps relevant to https://github.com/alecthomas/gometalinter/commit/eff485ae5abeeaa7b53b6a97e932c9974668bc10 too), megacheck adds flags for disabling individual tools

megacheck -h
...
  -simple.enabled
        Run gosimple (default true)
...
  -staticcheck.enabled
        Run staticcheck (default true)
...

[dmitshur]

What's the reason for wanting it? It's good for speed, is that what you're looking for? Or to close off the issue? Either is a good reason.

I thought this was going to be a non-controversial and easy enhancement opportunity. I did not expect that there would be downsides to replacing the 3 individual tools with megacheck.

I see now that you want to preserve different output from different tools separately, which makes that not the case.

As for why I want to do it, it's because I want to become more familiar with this project and try to contribute to it. Perhaps there's a better first issue for me to take then.

[bradleyfalzon]

I see now that you want to preserve different output from different tools separately, which makes that not the case.

Do you agree or disagree with this? Do you think there's value in keeping the existing behaviour, or do you think it should be swapped, considering the fact the original tool raising the issue would be obfuscated?

As for why I want to do it, it's because I want to become more familiar with this project and try to contribute to it. Perhaps there's a better first issue for me to take then.

I'm just not in love with the fact a simple swap would make it less clear which tool actually raised the issue, which is why I've been holding off doing this. I've kept it open, as I'd rather have this as an option for the user, so I'd probably prefer to hold off.

But if you don't see a problem with swapping it, then I'm all for it, it'd make the builds faster which is better for users and the hosted version.

[dmitshur]

Do you agree or disagree with this? Do you think there's value in keeping the existing behaviour, or do you think it should be swapped, considering the fact the original tool raising the issue would be obfuscated?

It's a little hard for me to say without bias, but I'm tempted to say that "I don't care which tool emitted a warning, I care about the content of the warning."

Locally, I've been using megacheck since it came out, immediately replacing the 3 individual tools. It never even occurred to me that I wasn't seeing which tools the messages came from, but that was already the case before, since my script simply ran vet, staticcheck, gosimple, unused, unconvert in sequence.

I rarely get output, so when I do, I'm very interested in seeing the content of the warning. From the content, I can tell what kind of an issue it is.

In other words, I let the content of the message drive my further decisions, and it doesn't seem that the name of the tool where the warning came from plays a significant role for my decision making.

Finally, the check name includes "SA", "S", "U" prefixes, so it's not impossible to know which tool it comes from, if needed.

However, your current UI is currently optimized for displaying messages separately from separate tools. So this wouldn't be as seamless a transition for your UI as it was for my UI (running all tools and merging their output).

[dmitshur]

I will say you should consider what it would be like to de-emphasize which tool a warning came from in your UI. If you decide that it's a good idea, this issue changes dramatically. (But this is a separate matter, worthy of discussion in a separate issue.)

It might make sense to have higher level categories though. Like, it might not matter so much whether a check came from vet or staticcheck, since they're both related to correctness with no false positives, but it would matter if a check came from lint or gosimple, since that's more of style/suggestions/optional simplification territory.

[dmitshur]

In the comment above, I said:

It might make sense to have higher level categories though. Like, it might not matter so much whether a check came from vet or staticcheck, since they're both related to correctness with no false positives, but it would matter if a check came from lint or gosimple, since that's more of style/suggestions/optional simplification territory.

I'm not sure if @dominikh saw this comment and that influenced his tool, of it was his plan all along anyway (more likely), but in https://github.com/dominikh/go-tools/issues/21#issuecomment-331061839 he mentioned a very relevant field:

Where severity is optional, and not fully defined yet, but will be used to differentiate compiler errors, suggestions (gosimple), warnings (most of staticcheck), high impact bugs and so on. We can standardize on a list later.

[dominikh]

(I'm following the conversation in this issue.)