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

[Merged by Bors] - small and mostly pointless refactoring #2934

Closed
wants to merge 11 commits into from

Conversation

danieleades
Copy link
Contributor

What is says on the tin.

This has got more to do with making clippy slightly more quiet than it does with changing anything that might greatly impact readability or performance.

that said, deriving Default for a couple of structs is a nice easy win

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Oct 8, 2021
@TheRawMeatball TheRawMeatball added C-Feature A new feature, making something new possible C-Code-Quality A section of code that is hard to understand or change and removed S-Needs-Triage This issue needs to be labelled labels Oct 8, 2021
@danieleades danieleades force-pushed the refactor/nit-pick branch 2 times, most recently from 2cad921 to ea9e0a0 Compare October 8, 2021 07:43
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

Reducing clippy warning noise is always nice!

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Oct 9, 2021
@mockersf
Copy link
Member

mockersf commented Oct 9, 2021

a lot of those changes are in bevy_render, and will be lost with the current rewrite

@danieleades
Copy link
Contributor Author

a lot of those changes are in bevy_render, and will be lost with the current rewrite

what's the action here? should we wait for that and rebase? or are you just warning me that all my hard work will be for naught?

@mockersf
Copy link
Member

a lot of those changes are in bevy_render, and will be lost with the current rewrite

what's the action here? should we wait for that and rebase? or are you just warning me that all my hard work will be for naught?

Not sure, I think bevy_render will be replaced by bevy_render2 which you can find in branch pipelined-rendering. I don't know how much of what you did can be ported, I am not following the rewrite very closely

@danieleades
Copy link
Contributor Author

squashed a few more lints. this repo is a pedant's dream

@TheRawMeatball
Copy link
Member

I'd rather not have #![allow(type_complexity)] in the code, as that lint gives too many false positives with bevy, and so we just disable it when running cargo clippy. Aside from that, it all looks good.

@IceSentry
Copy link
Contributor

IceSentry commented Oct 11, 2021

I'd rather not have #![allow(type_complexity)] in the code, as that lint gives too many false positives with bevy, and so we just disable it when running cargo clippy. Aside from that, it all looks good.

That approach doesn't really work for anyone that runs clippy on save in their editor. Personally I don't mind having it in the code, but I get why people might not like that.

Could we introduce a clippy.toml instead? type_complexity is already allowed in CI so having a clippy.toml simply makes it more obvious than having it hidden in a CI pipeline.

@danieleades
Copy link
Contributor Author

i don't mind. as soon as there's a consensus, i can update the PR

@TheRawMeatball
Copy link
Member

Does such a thing as a Clippy.toml exist? From a quick search, these seem to not quite exist: rust-lang/cargo#5034, which is why I didn't suggest it.

@IceSentry
Copy link
Contributor

I didn't look into it that much I just saw the beginning of the configuration section and assumed it would be enough https://github.com/rust-lang/rust-clippy#configuration

@IceSentry
Copy link
Contributor

Adding this to .cargo/config.toml seems to work

[target.'cfg(feature = "cargo-clippy")']
rustflags = ["-Aclippy::type_complexity"]

But that would required adding config.toml to the repository and I'm not sure if we want that, but it could at least be added to the recommended configs.

@IceSentry
Copy link
Contributor

So, from what I understand, clippy.toml is used for configuring lints and not allowing or denying specfic lints. Reading the doc for type_complexity https://rust-lang.github.io/rust-clippy/master/index.html#type_complexity it looks like it would be possible to simply increase the threshold that triggers the lint.

@danieleades
Copy link
Contributor Author

I tend to use clippy.toml mainly for configuring the MSRV (does bevy have a MSRV??)

Adding a compiler flag to a cargo.toml is no different to adding the flags inline in the lib.rs file(s), and the latter is usually preferred. Although in a big workspace project like this one, you'd have to duplicate the flags in every crate

@danieleades
Copy link
Contributor Author

There seems to be a few different solutions being thrown around in this thread, but what is the desired behaviour?

if it's to suppress the lint globally, i can just stick #![allow(clippy::type_complexity)] in the root.

@mockersf
Copy link
Member

New renderer has been merged, causing a lot of conflicts. Do you want to update this PR?

@mockersf mockersf removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Dec 16, 2021
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 17, 2022
@alice-i-cecile alice-i-cecile mentioned this pull request Jan 18, 2022
@danieleades danieleades force-pushed the refactor/nit-pick branch 2 times, most recently from c0c7ba7 to bf68d0f Compare January 18, 2022 19:57
@cart
Copy link
Member

cart commented Feb 3, 2022

Looks good to me! Resolve that last conflict and revert the iter::Copied change and we're good to go!

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

@cart I've done a fresh review pass and the changes you requested are complete :) This PR LGTM now.

@cart
Copy link
Member

cart commented Feb 13, 2022

bors r+

bors bot pushed a commit that referenced this pull request Feb 13, 2022
What is says on the tin.

This has got more to do with making `clippy` slightly more *quiet* than it does with changing anything that might greatly impact readability or performance.

that said, deriving `Default` for a couple of structs is a nice easy win
@bors bors bot changed the title small and mostly pointless refactoring [Merged by Bors] - small and mostly pointless refactoring Feb 13, 2022
@bors bors bot closed this Feb 13, 2022
@ghost ghost mentioned this pull request Feb 15, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Code-Quality A section of code that is hard to understand or change C-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants