-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
馃尡 enable godot on top-level comments #5591
馃尡 enable godot on top-level comments #5591
Conversation
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.
I think capitalization linting doesn't make sense as it has too many false positives.
The actual "trailing dot" part looks fine to me.
I would enable toplevel "trailing dot" linting but not capitalization.
# declarations - for top level declaration comments (default);
# toplevel - for top level comments;
# all - for all comments.
scope: toplevel
exclude:
- '^ \+.*'
- '^ ANCHOR.*'
WDYT?
P.S. We can test in a follow-up PR if all
is an option (trailing dot findings: top-level : 32, all: 1950).
This goes in the direction of more consistency across the code base, so I'm +1 |
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.
I agree the punctuation is more useful than the capitalization.
It's probably OK to enable "all", but the only concern I would have with that is it's not obvious to new developers coming in that the linting will flag comments starting with the name of a privately scoped variable (lowercase) as an issue, so it could cause some frustration when linting checks fail. Not a huge deal, but something to consider.
.golangci.yml
Outdated
exclude: | ||
- '^ \+.*' | ||
- '^ ANCHOR.*' | ||
capital: true |
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.
Do we need this? It seems flaky imo and lots of friction. The benefit of having a capital letter after a dot is small compared to a smoother new contributor experience
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.
I would drop capitalization as stated in: #5591 (review)
Just wanted to wait for additional opinions.
Just didn't want to directly fix it to visualize how bad it is :)
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.
Done
39b8f05
to
98e65ff
Compare
Dropped capitalization. |
/lgtm |
/lgtm |
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
Goal of the PR in its current state is to discuss if we want to enable godot also on toplevel comments (default is declarations).
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #