Skip to content
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

feat: remove v1 exclusion configuration #5451

Merged
merged 8 commits into from
Feb 21, 2025
Merged

Conversation

ldez
Copy link
Member

@ldez ldez commented Feb 19, 2025

Fixes #5298
Fixes #456

@ldez ldez added topic: cleanup Related to code, process, or doc cleanup area: exclusions labels Feb 19, 2025
@ldez ldez added this to the v2-unreleased milestone Feb 19, 2025
@ldez ldez marked this pull request as draft February 19, 2025 21:13
@ldez ldez force-pushed the feat/exclusions branch 3 times, most recently from 9cf68b2 to 78a202e Compare February 19, 2025 21:39
@ldez ldez force-pushed the feat/exclusions branch 2 times, most recently from 1a79d23 to 82aaf0d Compare February 19, 2025 22:54
@ldez ldez marked this pull request as ready for review February 19, 2025 23:11
@ldez ldez force-pushed the feat/exclusions branch 3 times, most recently from 5958c0e to 538fb13 Compare February 20, 2025 02:32
@ldez
Copy link
Member Author

ldez commented Feb 20, 2025

Debugging problems related to Windows that only happen on GitHub Actions is a hell

This is why I'm using Linux, it is faster, stable, and a file system that works as expected.

@ldez ldez force-pushed the feat/exclusions branch 2 times, most recently from 009e288 to 7dc37de Compare February 20, 2025 03:20
Copy link
Member

@alexandear alexandear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some minor grammar improvements.

Comment on lines 284 to 287
- comments
- stdErrorHandling
- commonFalsePositives
- legacy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe better is to use - as a delimiter between words instead of camelCase?

Suggested change
- comments
- stdErrorHandling
- commonFalsePositives
- legacy
- comments
- std-error-handling
- common-false-positives
- legacy

Or make preset names shorter with an extended description.

Copy link
Member Author

@ldez ldez Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to use camelCase because these are not field names.
And I want human-readable names, this is explained inside the issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I can vote, I vote to use kebab-case for human readable names. camelCase lacks the space usually seen in text and feels more aimed towards computers, similar with snake_case and underscore.

I know these are values and not keys, but we do use kebab-case in general in the config (e.g. disable-all and disable-dec-order-check) which I thought was because it was preferred (it is for me) and if so I suggest using the same convention for these values as well.

Copy link
Member Author

@ldez ldez Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will change the values but we agree those values have been documented inside the proposal and introduced by another PR. This PR is just adding them to the documentation.

My remark about human-readable names was related to the idea "make preset names shorter with an extended description", because I want to avoid the previous "short but unusable without the doc" names.

@ldez ldez merged commit 7bcac43 into golangci:main Feb 21, 2025
18 checks passed
@ldez ldez deleted the feat/exclusions branch February 21, 2025 13:08
@busser busser mentioned this pull request Mar 14, 2025
Closed
@ldez ldez modified the milestones: v2-unreleased, v2 Mar 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: exclusions topic: cleanup Related to code, process, or doc cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🌟 Let's talk about default exclusions please reconsider ignoring missing doc comments
4 participants