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

Add Default to Clippy Lints Lint groups #9616

Merged
merged 2 commits into from
Oct 11, 2022

Conversation

unvalley
Copy link
Contributor

@unvalley unvalley commented Oct 9, 2022

This PR adds a default (reset) button to Clippy Lints Lint groups. (change for website)
The page sets only Deprecated to false by default.
Certainly it is easy to set only deprecated to false, but it may be a bit lazy for beginners.

Screen.Recording.2022-10-10.at.17.58.52.mov

changelog: none

@rust-highfive
Copy link

r? @dswij

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 9, 2022
@flip1995
Copy link
Member

r? @xFrednet I think you are the best reviewer for this.

Should the Default option enable all lints that are warn-by-default, instead of all groups except for deprecated? That would've been what I expected.

The All option can also just set deprecated to false IMO.

@rust-highfive rust-highfive assigned xFrednet and unassigned dswij Oct 10, 2022
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, two smaller suggestions

$scope.resetGroupsToDefault = function () {
const groups = $scope.groups;
for (const [key, value] of Object.entries(GROUPS_FILTER_DEFAULT)) {
if (groups.hasOwnProperty(key)) {
Copy link
Member

Choose a reason for hiding this comment

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

This should always be true, shouldn't it?

util/gh-pages/index.html Show resolved Hide resolved
@xFrednet
Copy link
Member

Should the Default option enable all lints that are warn-by-default, instead of all groups except for deprecated? That would've been what I expected.

I think default should reset to the default of the lint list. If we only want it to be warn-by-default lints, then that should also be the default of the list IMO. With context, I think that this is already covered by the lint level selection. We could maybe add a second button, which is clippy::all 🤔

@unvalley
Copy link
Contributor Author

@xFrednet Thanks, I added a commit for your suggestion.

@flip1995
Copy link
Member

Should the Default option enable all lints that are warn-by-default, instead of all groups except for deprecated? That would've been what I expected.

I think default should reset to the default of the lint list. If we only want it to be warn-by-default lints, then that should also be the default of the list IMO. With context, I think that this is already covered by the lint level selection. We could maybe add a second button, which is clippy::all thinking

Makes sense to me. I don't think there is a need for a button for clippy::all, as long as we also have sorting by lint level (I forgot about this). So the original implementation of this PR SGTM.

@xFrednet
Copy link
Member

Looks good to me and runs perfectly. Thank you for the changes!

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 11, 2022

📌 Commit 178799f has been approved by xFrednet

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Oct 11, 2022

⌛ Testing commit 178799f with merge 122ae22...

@bors
Copy link
Collaborator

bors commented Oct 11, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 122ae22 to master...

@bors bors merged commit 122ae22 into rust-lang:master Oct 11, 2022
@unvalley unvalley deleted the add-default-to-lint-groups branch October 11, 2022 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants