-
Notifications
You must be signed in to change notification settings - Fork 13
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
Consider replacing staticcheck, gosimple, unused with a faster equivalent megacheck. #103
Comments
Yep agreed, this is better for users and for efficiency for me, but there's a few things I'd like to consider:
Either of these two options would then mask the underlying tool as just In the meantime, when I resolve #8, I might just add |
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 If this can be done now... Is this good material for a first contribution? I'd consider taking it if so. |
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.
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. |
to chime in here (perhaps relevant to alecthomas/gometalinter@eff485a too), megacheck adds flags for disabling individual tools
|
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 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. |
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?
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. |
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 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). |
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 |
In the comment above, I said:
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 dominikh/go-tools#21 (comment) he mentioned a very relevant field:
|
(I'm following the conversation in this issue.) |
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.
The text was updated successfully, but these errors were encountered: