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

New lint needless_as_bytes #13437

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

New lint needless_as_bytes #13437

wants to merge 1 commit into from
+199 −2

Conversation

samueltardieu
Copy link
Contributor

changelog: [needless_as_bytes]: new lint

Fix #13434

@rustbot
Copy link
Collaborator

rustbot commented Sep 21, 2024

r? @y21

rustbot has assigned @y21.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 21, 2024
LikeLakers2
LikeLakers2 approved these changes Sep 21, 2024
View reviewed changes
Copy link

@LikeLakers2 LikeLakers2 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, at least documentation-wise. Thank you for making the lint I suggested. :)

@bors
Copy link
Collaborator

bors commented Sep 22, 2024

☔ The latest upstream changes (presumably #13322) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez GuillaumeGomez added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 22, 2024
@samueltardieu samueltardieu force-pushed the issue-13434 branch 2 times, most recently from bffedc4 to 1867997 Compare September 22, 2024 17:21
GuillaumeGomez
GuillaumeGomez reviewed Sep 22, 2024
View reviewed changes
Copy link
Member

@GuillaumeGomez GuillaumeGomez left a comment

Choose a reason for hiding this comment

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

Apart from the missing ui test annotations, looks good to me, nice work!

@samueltardieu
Copy link
Contributor Author

I've added the missing test annotations, thanks for the review!

@samueltardieu
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Sep 22, 2024
@GuillaumeGomez
Copy link
Member

Looks all good to me, thanks! Let's just wait someone from the team to approve it too. :)

y21
y21 reviewed Sep 23, 2024
View reviewed changes
Copy link
Member

@y21 y21 left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me already, most of the comments I initially had were mentioned by others and are now resolved :)

clippy_lints/src/needless_as_bytes.rs Outdated Show resolved Hide resolved
tests/ui/needless_as_bytes.rs Show resolved Hide resolved
@samueltardieu
Copy link
Contributor Author

@y21 Do you see something that needs to be changed?

@y21
Copy link
Member

y21 commented Sep 26, 2024

I'll try to take another look later today

y21
y21 approved these changes Sep 26, 2024
View reviewed changes
Copy link
Member

@y21 y21 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

LikeLakers2
LikeLakers2 approved these changes Sep 26, 2024
View reviewed changes
Copy link

@LikeLakers2 LikeLakers2 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Not a team member, but figured I'd do one final approval of my own, to let you know that I don't see anything else I think needs fixing.

@y21
Copy link
Member

y21 commented Sep 26, 2024

FCP1 thread: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/FCP.3A.20needless_as_bytes

Footnotes

  1. final comment period, a process that all new lints need to go through where others can share "last minute" concerns ↩

@y21 y21 added S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 26, 2024
@samueltardieu
Copy link
Contributor Author

@y21 Any news?

@y21
Copy link
Member

y21 commented Oct 12, 2024

Sorry, have not had much time the last few days. There are some comments on the Zulip thread suggesting that this is something that some people do intentionally write to make it clear that it's the byte length (and this argument is also in the linked issue's drawbacks), so I'd want to see a poll on the Zulip for the category. I'll try to do it later today.

I'm personally in favor of this lint being warn-by-default, but we shouldn't have warn-by-default lints that people find hurts readability.

@LikeLakers2
Copy link

LikeLakers2 commented Oct 13, 2024 •
edited
Loading

@y21 I think it's also worth weighing the possibility of people simply not knowing string.len() == string.as_bytes().len(). Though, whether you'd be weighing it for or against warn-by-default, is up to you.

That said, as long as the lint exists in any capacity, I would be happy with the results.

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

@LikeLakers2 LikeLakers2 LikeLakers2 approved these changes

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

@y21 y21 y21 approved these changes

Assignees

@y21 y21

Labels
S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Complexity lint against some_string.as_bytes().len()
6 participants
@samueltardieu @rustbot @bors @GuillaumeGomez @y21 @LikeLakers2