Skip to content

Rename run_lints -> lints_enabled #7448

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 9, 2021
Merged

Rename run_lints -> lints_enabled #7448

merged 1 commit into from
Jul 9, 2021

Conversation

flip1995
Copy link
Member

@flip1995 flip1995 commented Jul 8, 2021

Just a quick rename of a utilities function. run_lints kinda suggested that the lints were run by this function. But the only thing this function does is to check if the lints are enabled in the context of the hir_id

changelog: none

@flip1995
Copy link
Member Author

flip1995 commented Jul 8, 2021

r? @camsteffen (GitHub UI suggested me your name 😄 )

@flip1995
Copy link
Member Author

flip1995 commented Jul 8, 2021

rust-highfive bot was down because of a invalid json file rust-lang/highfive#343

@camsteffen
Copy link
Contributor

camsteffen commented Jul 8, 2021 •
edited
Loading

Can we just remove it and use is_allowed instead? We're always passing a single Lint anyways. Maybe rename to is_lint_allowed. I've always thought is_allowed is too vague. But I think I'd prefer "allowed" to the opposite "enabled" because it refers to a specific Level. "enabled" doesn't really correspond to a rustc concept. Also run_lints is missing Level::ForceWarn.

@camsteffen
Copy link
Contributor

One more thought. We could have a variant that uses LateContext::last_node_with_lint_attrs, similar to span_lint. So we could have is_lint_allowed and is_lint_allowed_at.

@llogiq
Copy link
Contributor

llogiq commented Jul 9, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jul 9, 2021

📌 Commit 795868d has been approved by llogiq

@bors
Copy link
Contributor

bors commented Jul 9, 2021

⌛ Testing commit 795868d with merge 8da39e6...

@bors
Copy link
Contributor

bors commented Jul 9, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 8da39e6 to master...

@bors bors merged commit 8da39e6 into rust-lang:master Jul 9, 2021
@flip1995 flip1995 deleted the run_lints-rename branch July 9, 2021 12:36
@flip1995
Copy link
Member Author

flip1995 commented Jul 9, 2021

Can we just remove it and use is_allowed instead?

Oh I missed is_allowed when I added the run_lints function. I agree with your analysis and will rename is_allowed to is_lint_allowed and remove lints_enabled.

@flip1995 flip1995 mentioned this pull request Jul 9, 2021
Merged
bors added a commit that referenced this pull request Jul 9, 2021
@bors
Auto merge of #7449 - flip1995:remove-lints_enabled, r=camsteffen
cb16543 
Remove lints_enabled

r? `@camsteffen`

cc #7448 (comment)

I haven't added a variant with `last_node_with_lint_attrs` yet, since I didn't see a usecase for this. Also this field is not documented, so I'm wondering how it behaves with command line lints and so on.

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@camsteffen camsteffen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@flip1995 @camsteffen @llogiq @bors