How to Review a Pull Request (Linter Configuration)

[switchupcb]

(Credit: @gunnarmorling)

In an effort to improve slash commands, I've come across this: #856. There are 11 reviewers (@eaglemoor @CarsonHoffman @MrFlynn @iopred @chanbakjsd2 + more)... Yet when you try to implement slash commands, an example that is prone to security issues (and other code smells) is provided. This is because every maintainer was so focused on naming variables (important), and small stylistic changes (less important) that they forgot to ask questions about the actual code. Don't take this personal, @Strum355.

There are 40 pull requests, but most of them look like this: #1005. 7 months later... Nothing. It's understandable for you to miss pull requests. It's understandable to not check the repository every day. You aren't obligated to maintain the repository. However, when simple things like this go unmerged, it hints at inefficiency in your review approach. This post identifies the issues that cause this inefficiency and provides a solution to change this.

Creating an amazing environment for Go programmers in the Discord Go scene results in more resources and people that provide benefits to this library. It's in your best interest to make fixes and features easy to implement. In return, you receive better features, more performance (#959), and more maintainability.

@LaevusDexter, grab the popcorn.

What's the first issue?

Who here actually uses slash commands? If you are, please let me know because I look forward to crashing your bot. However, I don't actually believe I'll be crashing any bots because I don't believe you use slash commands. The main point being that there is no way anyone who uses a slash command bot is fine with using option indexes. Even if you use a map in your private codebase, it doesn't make sense to encourage what's already been explained here: #1126.

What's the next issue?

There are over 200 occurrences of naked returns and whatever is provided in the pull request below this post: I wonder how many issues can be solved by handling variables correctly. That's not a question... There is a 3000 line file with who knows what in it. The repository is full of unchecked errors, ineffectual assignments, and loads of unnecessary err =,\n if err statements. There are over 40 TODO statements and more — incorrectly labeled — deprecated ones... This is the worst Go codebase I've ever read. I'm not even making this statement as an insult. It's really the worst. Luckily, we can fix this.

Solution 1

The first solution starts with standardizing the codebase. The issue with @Atrox's statement about @FedorLap2006 following projet guidelines is that there are no guidelines: golint has been deprecated for a while... I've added basic linter settings that can be used to ensure no stylistic bonanzas in PRs. That includes a style preset (that bans naked returns), misspell (to assist foreign speakers), and other linters. I've created a pull request to address these but there is too many issues for one person to fix. After these issues are fixed, all a potential contributor will have to do is run golangci-lint run --fix before creating a pull request.

However, there are still some questions we need to answer: For example, do we enforce named return variables in the code? Go even states themselves:

Naked return statements should be used only in short functions, as with the example shown here. They can harm readability in longer functions.

Do we use whitespace linter (wsl), magic number detector (gomnd), lll, wrapcheck, or any of these linters? Error checks and gosimple would be great. Everything with the # Recommended label in golanci.yml in #1131 would be amazing.

Solution 2

The next solution requires a line to be drawn on how tagging is done. Go modules use tags for a reason. Semver is a standard that can be used to label versions. Using both eliminates issues with backwards compatibility, but that shouldn't be an issue right now. Why are we using Go 1.13 (from 2019)? It's 2022 and Go 1.18 is around the corner.

Deprecating functions is fine, but avoiding features that "would be good but <insert reason equal to we've always done it this way>" is NOT GOOD. Don't use a pen and paper in the era of calculators. Don't be discord.py. Features like Application Commands (April, 2022) are coming and Discord isn't going to stop them just because a non-professional has to change his code.

I'm not saying that this has happened in this repository, but that it's a direction we should not encourage.

Note

Don't take the post personally. I could be artificial ("nice") but there's no point. You either understand what I've explained here, or you lay butt up with your head in the sand. Do we pull? Do we fork? Find out on the next episode of Discordgo.

[bwmarrin]

First, I'd like to thank you for the time you may have put into this. Some of the issues you've brought up are definitely issues with this library and talking about them and future goals isn't a bad thing. However, this isn't a specific bug/feature and as such should be moved to the Github Discussions :) So if you'd like to continue this please start up a discussion there.

EDIT: I moved this to a discussion :)

[switchupcb]

[FedorLap2006]

If you want to attach the related PRs to this disucssion I think would be better to put them into description under "Linked PRs" section.

[switchupcb]

#1005 can be closed unmerged, since the logo is vectorized as of 25 days ago. There are some stale pulls from 2 years ago that can be triaged to prevent issues in #982