Enable lints #191
Enable lints #191
Conversation
|
already did some fixes |
There was a problem hiding this comment.
Thanks for the quick response! After reviewing, most of the changes are reasonable. I think the #[must_use] is especially helpful for users who might mistakenly do this:
let mut s = "hello, world";
s.green(); // must_use raise for this
println!("{s}");My main concern is with the change to const fn. What do you think?
A small bit of feedback on making formatting PRs: IMO, it's better to have one commit that sets the rules, and a second commit to actually apply them, instead of both setting rules and formatting in one commit. This allows:
- the reviewer to see what errors would get raised before formatting is applied, by checkout out the pre-format commit.
- one to create a
.git-blame-ignore-revsentry to ignore formatting changes without ignoring rule changes.
It's easier to squash multiple commits into one than to un-squash one commit into multiple.
But this is just a small nitpick, and a personal preference mostly, so don't worry about it for this PR 🙂
| @@ -168,7 +169,7 @@ mod specs { | |||
| fn clicolor_behavior() { | |||
| rspec::run(&rspec::describe("ShouldColorize", (), |ctx| { | |||
| ctx.specify("::normalize_env", |ctx| { | |||
| ctx.it("should return None if error", |_| { | |||
| ctx.it("should return None if error", |()| { | |||
There was a problem hiding this comment.
Using |_| is the documented usage for rspec, per their README. What was the linting rule (and its reason) for switching from _?
There was a problem hiding this comment.
There was a problem hiding this comment.
Alright, makes sense. In summary, if rspec changes their usage and starts passing a value instead of (), we'll know about it through a compilation error. Works for me!
|
Thanks for the work! I'll leave this PR open for a week or two to give any of the other maintainers a chance to chime in if they choose. Feel free to ping me if you feel like I'm leaving this PR open too long 🙂 |
No description provided.