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 #404

Merged
merged 2 commits into from
May 15, 2024
Merged

Fix clippy lints #404

merged 2 commits into from
May 15, 2024

Conversation

P1n3appl3
Copy link
Contributor

I use a global rustfmt config which cause spurious changes when formatting in this repo. Adding an empty .rustfmt.toml in the root will force rustfmt to use the default options for line length/comment reflow/etc.

I also wanted to run clippy while making some other changes and cleaned up the existing lints so I could see any new ones I added. The only interesting one was result_large_err in the typechecker where the Ok variants were around 32 bytes and some of the Errs were a couple hundred bytes. I just allowed it for that whole module but I could go allow the individual functions it fired on, or use boxing to make the Result smaller.

@sharkdp
Copy link
Owner

sharkdp commented Mar 19, 2024

I use a global rustfmt config which cause spurious changes when formatting in this repo. Adding an empty .rustfmt.toml in the root will force rustfmt to use the default options for line length/comment reflow/etc.

I don't have this in any of my Rust projects, and I'd rather avoid it if possible.

@P1n3appl3 P1n3appl3 changed the title Add empty rustfmt config and fix clippy lints Fix clippy lints Mar 19, 2024
@P1n3appl3
Copy link
Contributor Author

Removed it, would you be ok with adding it as an entry in .gitignore so I can have it locally without worrying about committing it? Without it I'd accidentally run cargo fmt and end up introducing a bunch of spurious changes that line-wrap comments because I use wrap_comments = true

@triallax
Copy link
Contributor

No opinion on whether to have .rustfmt.toml in our .gitignore, but you can put it in a global .gitignore file that applies to all repos on your system: https://sebastiandedeyne.com/setting-up-a-global-gitignore-file/

@P1n3appl3
Copy link
Contributor Author

Good point, I'll just add it to my global ignore

@triallax
Copy link
Contributor

Closing since people can use a global .gitignore.

@triallax triallax closed this Mar 20, 2024
@triallax
Copy link
Contributor

Ah, sorry, I didn't pay attention and thought this PR just added .rustfmt.toml to .gitignore...

@triallax triallax reopened this Mar 20, 2024
Copy link
Contributor

@triallax triallax left a comment

Choose a reason for hiding this comment

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

LGTM

@sharkdp
Copy link
Owner

sharkdp commented Mar 20, 2024

I also wanted to run clippy while making some other changes and cleaned up the existing lints so I could see any new ones I added. The only interesting one was result_large_err in the typechecker where the Ok variants were around 32 bytes and some of the Errs were a couple hundred bytes. I just allowed it for that whole module but I could go allow the individual functions it fired on, or use boxing to make the Result smaller.

I saw those clippy warnings before and wasn't yet sure how to handle these. I think we should more carefully look into whether or not those appear in performance critical code. And if yes, whether or not boxing the error type would be beneficial. Otherwise, I'm fine with allowing it.

@P1n3appl3
Copy link
Contributor Author

Not sure why the doc checks failed, re-running book/build.sh doesn't introduce any changes. I rebased onto master in case it had to do with being out of date.

Copy link
Contributor

@hamirmahal hamirmahal left a comment

Choose a reason for hiding this comment

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

It's possible to add an optional cargo clippy check, by making all other checks required, like CICD / all-jobs in sharkdp/bat#2913.

Even if a new cargo clippy check is optional, it still might encourage contributors to commit fewer linting errors into the codebase when they see a failing clippy check for their PR. clippy usually says how to fix any errors, too.

image

@hamirmahal
Copy link
Contributor

The previous comment is in reference to #355 (comment), but I thought I'd bring it up here, since it's relevant again.

@@ -694,6 +694,7 @@ pub fn tokenize(input: &str, code_source_id: usize) -> Result<Vec<Token>> {
}

#[cfg(test)]
#[allow(clippy::type_complexity)]
Copy link
Owner

Choose a reason for hiding this comment

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

Happy to merge this PR if we can remove these excepts. Here and below.

@sharkdp sharkdp merged commit 0b2232a into sharkdp:master May 15, 2024
15 checks passed
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.

4 participants