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.

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

Add large_include_file lint #8727

Merged
merged 1 commit into from
Apr 25, 2022
Merged

Add large_include_file lint #8727

merged 1 commit into from
Apr 25, 2022

Conversation

Serial-ATA
Copy link
Contributor

changelog: Add [large_include_file] lint
closes #7005

@rust-highfive
Copy link

r? @xFrednet

(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 Apr 21, 2022
@Serial-ATA Serial-ATA force-pushed the lint-large-includes branch 2 times, most recently from 275005c to 9d4e8ca Compare April 22, 2022 21:15
xFrednet
xFrednet reviewed Apr 24, 2022
View reviewed changes
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.

This version already looks good to me. A few small NITs that should all be simple to fix 🙃

tests/ui-toml/large_include_file/large_include_file.rs Show resolved Hide resolved
tests/ui-toml/large_include_file/too_big.txt Outdated Show resolved Hide resolved
clippy_lints/src/large_include_file.rs Outdated Show resolved Hide resolved
clippy_lints/src/large_include_file.rs Show resolved Hide resolved
@Serial-ATA Serial-ATA force-pushed the lint-large-includes branch from 9d4e8ca to a85dc87 Compare April 24, 2022 14:09
@xFrednet
Copy link
Member

LGTM, thank you for this lint!

@bors r+

@bors
Copy link
Contributor

bors commented Apr 25, 2022

📌 Commit a85dc87 has been approved by xFrednet

@bors
Copy link
Contributor

bors commented Apr 25, 2022

⌛ Testing commit a85dc87 with merge 760f293...

@bors
Copy link
Contributor

bors commented Apr 25, 2022

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

@bors bors merged commit 760f293 into rust-lang:master Apr 25, 2022
This was referenced Apr 25, 2022
Merged
Closed
@lopopolo
Copy link

lopopolo commented Apr 29, 2022 •
edited
Loading

Hi folks, is this a lint that can be moved to restriction rather than pedantic? It seems similar in spirit to the restriction lints in that there is nothing inherently wrong with using include str and include bytes with large contents, but for certain cases or project types you may wish to lint.

@xFrednet
Copy link
Member

Moving it to the restriction group sounds good to me. The group was actually one thing I was unsure about, but it slipped through the review 😅

@Serial-ATA
Copy link
Contributor Author

The lint is already restriction 🤔

@xFrednet
Copy link
Member

You're right, I didn't look up what group it currently is in. 😅

@lopopolo
Copy link

Oh, it is in restriction. Sorry I didn't look at the code that closely. The linked ticket mentioned pedantic.

Thanks folks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xFrednet xFrednet xFrednet left review comments

Assignees

@xFrednet xFrednet

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Lint on large std::{include_str!, include_bytes!}; calls.
5 participants
@Serial-ATA @rust-highfive @xFrednet @bors @lopopolo