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

fix a couply of clippy findings #6007

Merged
merged 2 commits into from
Jan 6, 2024

Conversation

matthiaskrgr
Copy link
Member

No description provided.

@ytmimi
Copy link
Contributor

ytmimi commented Jan 5, 2024

I think there's some overlap here with what's being proposed in #5400

src/bin/main.rs Outdated
Comment on lines 389 to 395
fn should_print_with_colors<T: Write>(session: &mut Session<'_, T>) -> bool {
match term::stderr() {
Some(ref t)
if session.config.color().use_colored_tty()
matches!(term::stderr(), Some(ref t) if session.config.color().use_colored_tty()
&& t.supports_color()
&& t.supports_attr(term::Attr::Bold) =>
{
true
}
_ => false,
}
&& t.supports_attr(term::Attr::Bold))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

#5400 is using is_some_and, which I think it a little clearer than the matches! that you're using here.

    term::stderr().is_some_and(|t| {
        session.config.color().use_colored_tty()
            && t.supports_color()
            && t.supports_attr(term::Attr::Bold)
    })

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@matthiaskrgr
Copy link
Member Author

looking at the commits of #5400 , I skipped most of these warnings because I didn't find them very interesting but yes there might be some overlap besides that.

@ytmimi
Copy link
Contributor

ytmimi commented Jan 6, 2024

Out of curiosity, what about these changes did you find interesting?

@matthiaskrgr
Copy link
Member Author

I'm usually trying to go for stuff that imo makes code slightly more elegant, simpler or easier to understand (for me, at least 🙃 ) / doing the same things with less code, ĺike using an array instead of vec when its sufficient, having a nice flow of iterator chains that help building a metal model what is going on etc.

Unnecessary needles borrows which I didnt touch can sometimes help keep in mind that a var is borrowed, even if it is already a reference (although I think rust-analyzer can show you that nowadays..?)

IIRC Caleb once mentioned that he liked the if nesting as is, which is why I didn't touch collapsible_else_if (this is kind of a personal-taste lint, sometimes it makes sense, but not always imo.

Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

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

Thanks for explaining. Overall I think these changes look reasonable!

@ytmimi ytmimi merged commit 75e3172 into rust-lang:master Jan 6, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants