-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Add comments in user config struct #3040
Conversation
9a13b9a
to
27be735
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. I only skimmed the comments, don't have time to proof-read everything, but I'm sure they are good.
I think we should merge this soon because it conflicts with other changes to UserConfig.
@@ -20,7 +20,8 @@ linters: | |||
linters-settings: | |||
exhaustive: | |||
default-signifies-exhaustive: true | |||
|
|||
staticcheck: | |||
checks: ["all", "-SA1019"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this? Did you include this on purpose? If so, might be useful to make this a separate commit and explain why this is added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes. See 3e2ef84
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes. See 3e2ef84
We're going to make this user config struct a more authoritative source of truth. Some of these fields weren't actually explained anywhere so I've added explanations. In places where a lot of explanation is required I've linked to existing explanations in other docs.
This is pretty funny: the staticcheck linter gets mad if we use a field which is marked in a comment as being deprecated. But it tripped on my own comment saying that a field is deprecated in terms of the user config! Obviously we have to make use of this field, otherwise we would just remove it entirely rather than mark it as deprecated, so I'm silencing this lint. I doubt this lint would actually come in handy in other cases (like when using a third party package) and worst case scenario we just end up fixing the problem when we try to upgrade the package and the deprecated field is now gone).
27be735
to
3e2ef84
Compare
We're going to make this user config struct a more authoritative source of truth.
Some of these fields weren't actually explained anywhere so I've added explanations.
In places where a lot of explanation is required I've linked to existing explanations in other docs.
PR Description
Please check if the PR fulfills these requirements
go run scripts/cheatsheet/main.go generate
)docs/Config.md
) have been updated if necessary