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

Remove needless as_bytes() call #2147

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

Conversation

samueltardieu
Copy link

String::len() already returns the number of bytes in the string.

`String::len()` already returns the number of bytes in the string.
Copy link

codecov bot commented Sep 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.03%. Comparing base (7c0024a) to head (bed207e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2147      +/-   ##
==========================================
- Coverage   97.12%   97.03%   -0.10%     
==========================================
  Files         151      151              
  Lines       20101    19602     -499     
  Branches      447      447              
==========================================
- Hits        19524    19020     -504     
- Misses        482      487       +5     
  Partials       95       95              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@briansmith
Copy link
Owner

Thanks for the PR!

String::len() already returns the number of bytes in the string.

This is true but I would argue that the way the code is currently written is clearer and perhaps {str/String}::len() shouldn't exist. Since the current code passes Clippy I am inclined to leave it as-is and I hope Clippy doesn't start warning about this pattern.

WDYT?

@samueltardieu
Copy link
Author

I totally agree that {String, str}::len() should not exist in the first place. But they do, and I think using .as_bytes().len() adds to the confusion, not at the place where it is used, but everywhere else: it may let people think that this is necessary, and that len() alone would return the number of codepoints.

This is why I prefer to use the function directly and have a clippy lint (which can be disabled of course, at a local or project level) to warn people that this is unnecessary because precisely len() is weirdly defined on strings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants