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] - Fix some nightly clippy lints #2522

Closed
wants to merge 1 commit into from

Conversation

BoxyUwU
Copy link
Member

@BoxyUwU BoxyUwU commented Jul 22, 2021

on nightly these two clippy lints fail:

@github-actions github-actions bot added S-Needs-Triage This issue needs to be labelled S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work labels Jul 22, 2021
@mockersf
Copy link
Member

mockersf commented Jul 23, 2021

I think all the #[allow(clippy::unused_unit)] are due to all the macros for tuple running on tuples of size 0 which... are not actually tuples but just unit.
What would be the impact of fixing that so the macros start at 1?

@mockersf mockersf added 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 S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work labels Jul 23, 2021
@BoxyUwU
Copy link
Member Author

BoxyUwU commented Jul 23, 2021

  • fn foo() would not be a valid system, not sure if this is actually useful or not but it doesnt seem harmful to allow
  • () would not be a valid fetch making Query<()> invalid so we wouldnt be able to do filter only queries
  • () would not be a bundle... which actually seems like a win :)

@mockersf
Copy link
Member

mockersf commented Jul 23, 2021

so... I think we should stop considering unit to be a special case of tuples, have the macros start at 1, and only impl the relevant trait for unit

@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@mockersf mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 24, 2021
@alice-i-cecile
Copy link
Member

so... I think we should stop considering unit to be a special case of tuples, have the macros start at 1, and only impl the relevant trait for unit

I agree with this.

fn foo() would not be a valid system, not sure if this is actually useful or not but it doesnt seem harmful to allow

I've used this for testing and debugging before, so I'd like to keep it.

() would not be a valid fetch making Query<()> invalid so we wouldnt be able to do filter only queries

I would also like to keep this.

() would not be a bundle... which actually seems like a win :)

Yeet this one.

@mockersf
Copy link
Member

@TheRawMeatball mentioned that unit are actually tuples.

@BoxyUwU
Copy link
Member Author

BoxyUwU commented Jul 25, 2021

they are, i still think it makes sense to split out these impls though as the reason for having them is different than the impls for other tuples

@TheRawMeatball
Copy link
Member

I'd disagree - having the tuple implementing ones also impl for the unit type is rather elegant imo, and I think we should keep them. IMO, to fix the lints, we should just add #[allow(clippy::unused_unit)] to these macros.

@alice-i-cecile alice-i-cecile added the P-High This is particularly urgent, and deserves immediate attention label Jul 29, 2021
@cart
Copy link
Member

cart commented Jul 29, 2021

I agree with @TheRawMeatball: a separate manual impl for unit is cumbersome / redundant / error prone. We should just suppress the lint for relevant impl macros (if there is no way to phrase the code to suppress the lint implicitly).

An example of an implicit suppression:

fn foo(_bar: T) {
  // bar isn't used and _ suppresses the unused lint
}

@cart
Copy link
Member

cart commented Jul 29, 2021

() no longer a bundle

I'm cool with this

edit: im cool with this now that direct spawn() calls are a thing.
However this should probably be a separate decision that we shouldn't make in the heat of the moment. Lets suppress this in the best way we can and handle it later if anyone feels strongly about it.

() no longer a Fetch

This creates problems for both filterless queries and fetchless filters (because they both rely on () having a Fetch impl ... filters are a special case of Fetch). This should stay as-is.

fn foo() would not be a valid system

strong disagree. users should be able to create inputless systems (for things like printing messages)

@BoxyUwU BoxyUwU force-pushed the nightly-clippy-lints branch from b17f4aa to cc2eeec Compare July 29, 2021 20:19
@cart
Copy link
Member

cart commented Jul 29, 2021

Sorry I added a new thought in the edit of my previous message, which probably should have been a new message so yall don't miss it.

@BoxyUwU BoxyUwU force-pushed the nightly-clippy-lints branch from e3ece27 to 2246fa0 Compare July 29, 2021 20:23
@BoxyUwU
Copy link
Member Author

BoxyUwU commented Jul 29, 2021

imo () being a bundle never has a use case so having it implemented can only cause silent errors but im fine to table this for a different PR

@cart
Copy link
Member

cart commented Jul 29, 2021

bors r+

bors bot pushed a commit that referenced this pull request Jul 29, 2021
@bors bors bot changed the title Fix some nightly clippy lints [Merged by Bors] - Fix some nightly clippy lints Jul 29, 2021
@bors bors bot closed this Jul 29, 2021
cart pushed a commit that referenced this pull request Jul 30, 2021
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 P-High This is particularly urgent, and deserves immediate attention
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants