Skip to content

Fix clippy lints #73

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 9 commits into from
Oct 12, 2020
Merged

Fix clippy lints #73

merged 9 commits into from
Oct 12, 2020

Conversation

Zenithsiz
Copy link
Contributor

This PR fixes some clippy lints existing in the crate and adds #![warn(...)]s for others.

Also I'm not sure this PR looks right in terms of commits and merges, I tried my best to be up to date with upstream/master, but I think I ended up doing worse. I'm not sure how to fix it, but if necessary, I'll try my best to make this look better.

This PR also depends on the PR I made earlier on unsafe for documentation, although I didn't mean for this, I'm just new to git and thought a new branch by default would go off master and not the current branch.

@tomprogrammer
Copy link
Owner

You could rebase this PR on master instead of your previous PR by executing git rebase --onto master unsafe-doc fix-clippy-lints. Unfortunately this results in quite a few conflicts because the order or formatting of the code changed in the unsafe-doc branch. It's ok for me if you leave this PR like it is. If you nevertheless want to try, remember that you can back out with git rebase --abort. If you could resolve all conflicts finish with git rebase --continue.

@Zenithsiz
Copy link
Contributor Author

I'll try that then, thank you! Sorry for the trouble.

@Zenithsiz
Copy link
Contributor Author

I believe I managed to rebase it, but I had a warning telling me to do git pull, which seems to have merged unsafe-doc into this branch again, should I rebase again and do a git push -f?

@Zenithsiz
Copy link
Contributor Author

Okay, it should be fine now, I reverted last commit that merged the unsafe-doc branch back

tomprogrammer
tomprogrammer reviewed Oct 11, 2020
View reviewed changes
@Zenithsiz
Copy link
Contributor Author

Sorry for replying in a new comment, I saw the suggest change you left on replacing it with a static_assert, but I can't find it here on the github page for some reason.
Regarding it, by doing it with values and using assert_eq, we also guarantee not only the impls exist, but also that their behaviours are what we expect (at least for the string "A"), but it could be split into 2 tests in the future, one checking the existence of these impls, and another checking their implementation (if even required, the implementations are 1 liners, so might be too much to make tests for them).

@tomprogrammer
Copy link
Owner

Ah, you've seen that comment. I thought I removed it early enough after realizing the same that you wrote. Next time I should keep the comment and just edit it I guess. :)

@Zenithsiz
Copy link
Contributor Author

Oops, sorry, I thought I just couldn't find it or maybe github wasn't updating the comment for some reason

Zenithsiz and others added 5 commits October 11, 2020 21:08
@Zenithsiz
Fixed most existing clippy lints and now warning against some `peda…
cb19d68 
…ntic` clippy lints.
@Zenithsiz
Added some clippy restriction lints.
5fffc7e 
Changed most indexing to `get_unchecked` because of lint `clippy::indexing_slicing`. This should also improve performance, as the compiler doesn't need to emmit panic code.
`clippy::option_if_let_else` is now allowed throughout the whole crate, as it doesn't add much.
@Zenithsiz
Changed closure arguments from |ascii| ... to |ch| ... to match w…
26b0c90 
…ith master
@Zenithsiz
Fixed ascii_str::tests::generic_as_ascii_str's mutable reference te…
8d239be 
…sts and added tests within it for most expected types.
@Zenithsiz
Update src/ascii_str.rs
627891c 
Co-authored-by: Thomas Bahn <thomas@thomas-bahn.net>
@Zenithsiz
Copy link
Contributor Author

Ah, some of the changes still don't work on 1.41.1. I'll attempt to go fix them.

tomprogrammer
tomprogrammer requested changes Oct 11, 2020
View reviewed changes
@Zenithsiz
Fixed issue regarding usage of subslice patterns.
418de00 
Fixed usage of inexistant `AsMut<str>` impl for `String` (Only added in `1.43`).
Extended test `ascii_str::tests::lines_iter_rev`.
@Zenithsiz
Fixed usage of String in a #[no_std] environment in `ascii_str::t…
78d9f48 
…ests`.
@Zenithsiz
Fixed usage of std instead of core in a `ascii_str::tests::generi…
d1edb56 
…c_as_ascii_str`.
@tomprogrammer
Copy link
Owner

Thanks, great work!

@tomprogrammer tomprogrammer merged commit 5bf5bcd into tomprogrammer:master Oct 12, 2020
@Zenithsiz Zenithsiz deleted the fix-clippy-lints branch October 12, 2020 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomprogrammer tomprogrammer tomprogrammer requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Zenithsiz @tomprogrammer