-
Notifications
You must be signed in to change notification settings - Fork 82
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
Fix various warnings and issues #79
Conversation
Might be worth adding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not at ease with the Style::contains change, I'd like a rationale and a use case for that. Also it's a breaking change so it need a major version bump.
@@ -96,7 +96,7 @@ impl Style { | |||
/// assert_eq!(colored.style().contains(Styles::Italic), true); | |||
/// assert_eq!(colored.style().contains(Styles::Dimmed), false); | |||
/// ``` | |||
pub fn contains(&self, style: Styles) -> bool { | |||
pub fn contains(self, style: Styles) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure about this change ? I don't think that consuming the style for a query is wise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also confused by this initially, but according to rust this is actually more efficient because the size of passing around Style
is smaller than the size of passing around a reference to it:
take a look at this output:
warning: this argument (1 byte) is passed by reference, but would be more efficient if passed by value (limit: 8 byte)
--> src/style.rs:99:21
|
99 | pub fn contains(&self, style: Styles) -> bool {
| ^^^^^ help: consider passing by value instead: `self`
|
= note: `#[warn(clippy::trivially_copy_pass_by_ref)]` on by default
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#trivially_copy_pass_by_ref
I guess the point is that because this enum hs the Copy
trait, its cheaper to copy and pass the value then it is to pass around a reference to the original.
take a look here for details: https://rust-lang.github.io/rust-clippy/master/#trivially_copy_pass_by_ref
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting ! TIL ! :)
@@ -321,7 +321,7 @@ impl ColoredString { | |||
/// assert_eq!(cstr.bgcolor(), None); | |||
/// ``` | |||
pub fn bgcolor(&self) -> Option<Color> { | |||
self.bgcolor.as_ref().map(|x| *x) | |||
self.bgcolor.as_ref().copied() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL 👍
@@ -284,6 +281,7 @@ mod tests { | |||
} | |||
} | |||
|
|||
#[test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Yes, I think adding a clippy test to the CI would be a great idea ! |
I'll add it in tonight :) |
c0d4169
to
4a03efc
Compare
Added clippy to Travis in 4a03efc. I've also enabled caching on travis for faster build times |
4a03efc
to
09f13d1
Compare
09f13d1
to
7390472
Compare
@mackwic any chance this could get reviewed / merged when you have the time? |
Thank you for the ping ! It's all good ! :D Thank you for your work and the time you put it in ! 🙏 |
Per this project policy, as we just merged a significant work on colored, you are granted the title of contributor and committer ! Congrats ! 🎊 🎉 Now, you have the authority to review and merge PRs of other contributors. :) Obviously, you don't have to do anything more than before. It's not an obligation and you can carry on your life as before. Please don't see it as an obligation but rather as a new possibility, if and only if you want to push this crate forward. No pressure. 🙏 |
I've listed the details of each fix into its own commit message
Warnings were found by running
cargo clippy
and
cargo test
rust version 1.43.0
@kurtlawrence