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 Clippy lints #1661

Merged
merged 7 commits into from
May 27, 2021
Merged

Fix Clippy lints #1661

merged 7 commits into from
May 27, 2021

Conversation

mohamed-abdelnour
Copy link
Contributor

The Minimum Supported Rust Version (MSRV) for bat is 1.45.0. Setting msrv = "1.45.0" in .clippy.toml (not part of the repository) and running cargo clippy emits 7 warnings. These are proposed fixes to said warnings. If there are any lints we would rather allow, I can revert the relevant commit(s) and add the appropriate lint attribute(s).

Also, do you think we should create .clippy.toml/clippy.toml and set the MSRV to ensure consistency amongst contributors?

@mohamed-abdelnour
Copy link
Contributor Author

Forgot to ask, do I update the CHANGELOG for something like this? If so, does it go under the "Other" heading?

@Enselic
Copy link
Collaborator

Enselic commented May 21, 2021

Thanks for taking the time to make another contribution!

Seems like we can't add msrv in .clippy.toml to the repo, because it was introduced in Rust 1.50.0 (commit) and will cause an error with Rust 1.45.0:

% cargo clippy         
    Checking bat v0.18.1 (/home/martin/src/bat)
error: error reading Clippy's configuration file `/home/martin/src/bat/.clippy.toml`: unknown field `msrv`, expected one of ...

error: aborting due to previous error

error: could not compile `bat`.

To learn more, run the command again with --verbose.

I suspect that latest Rust clippy is smarter than in 1.45.0 and therefore finds more lints.

I took a quick look at your changes, and they all seemed reasonable, but I need some more time to make an in-depth code review.

I personally think it is fine to mention this in CHANGELOG.md under Other.

@mohamed-abdelnour
Copy link
Contributor Author

Thank you for your review!

Seems like we can't add .clippy.toml to the repo, because it was introduced in Rust 1.50.0 (commit) and will cause a compilation error with Rust 1.45.0:

Got it, maybe in the future when the MSRV is bumped to 1.50.0.

I took a quick look at your changes, and they all seemed reasonable, but I need some more time to make an in-depth code review.

Take all the time you need!

Given that the lints are not showing up on 1.45.0, as a point of reference, I'll post links to the lints and their relevant commit(s). Hopefully this helps with the review process. Links are in the format:

  • Clippy lint
    • Relevant commit(s)

Clippy Lints

src/pager.rs Outdated
Comment on lines 101 to 115
// Is false if the given expression does not match any of the
// patterns; this ensures 'less' is never silently used if BAT_PAGER
// or --pager has been specified.
let use_less_instead = matches!(
(&source, &kind),
// 'more' and 'most' do not supports colors; automatically use
// 'less' instead if the problematic pager came from the
// generic PAGER env var
(PagerSource::EnvVarPager, PagerKind::More)
| (PagerSource::EnvVarPager, PagerKind::Most)

// If PAGER=bat, silently use 'less' instead to prevent
// recursion ...
| (PagerSource::EnvVarPager, PagerKind::Bat)
);
Copy link
Owner

@sharkdp sharkdp May 22, 2021

Choose a reason for hiding this comment

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

I personally think this is quite a bit harder to read now. The previous match isn't great either. Maybe this should be written with an outer if clause instead?

let use_less_instead = if source == PagerSource::EnvVarPager {
  // full comment here
  matches!(&kind, PagerKind::More | PagerKind::Most | …)
} else {
  false
};

Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

The other changes look good - thank you!

@mohamed-abdelnour
Copy link
Contributor Author

mohamed-abdelnour commented May 22, 2021

Thank you!

Maybe this should be written with an outer if clause instead?

I agree, this is much better.

matches!(&kind, PagerKind::More | PagerKind::Most | …)

Just checking, kind does not need to be referenced here, given that we're not creating a tuple anymore and it would be automatically dereferenced after rust-lang/rfcs#2005, correct?

@mohamed-abdelnour
Copy link
Contributor Author

On another note, I promise I'm not trying to rush you or anything, I am just new here and wanted to know what is more convenient for you. I was working on another issue and wanted to discuss it so I added a couple of comments to the issue; is that easy for you to spot and review, or should I create a PR and have the discussion there?

@sharkdp
Copy link
Owner

sharkdp commented May 27, 2021

Just checking, kind does not need to be referenced here, given that we're not creating a tuple anymore and it would be automatically dereferenced after rust-lang/rfcs#2005, correct?

Right. Didn't test my code :-)

On another note, I promise I'm not trying to rush you or anything, I am just new here and wanted to know what is more convenient for you. I was working on another issue and wanted to discuss it so I added a couple of comments to the issue; is that easy for you to spot and review, or should I create a PR and have the discussion there?

No. Discussion in the ticket is completely fine. Comments should be visible to everyone watching the repo. We just didn't have the time to answer, yet.

@sharkdp sharkdp merged commit aa74d19 into sharkdp:master May 27, 2021
sharkdp pushed a commit that referenced this pull request May 27, 2021
@mohamed-abdelnour
Copy link
Contributor Author

No. Discussion in the ticket is completely fine. Comments should be visible to everyone watching the repo. We just didn't have the time to answer, yet.

Got it, take as much time as you need! Thank you for being patient will all my questions; I appreciate it! :)

@mohamed-abdelnour mohamed-abdelnour deleted the fix-linting-warnings branch May 27, 2021 16:55
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.

3 participants