Skip to content

Conversation

DaAlbrecht
Copy link
Collaborator

@DaAlbrecht DaAlbrecht commented May 8, 2025

Adds a new lint called camera_modification_in_fixed_update that checks if systems are added to the FixedUpdate schedule that mutably query entities with a Camera component.

closes #105

@DaAlbrecht DaAlbrecht added A-Linter Related to the linter and custom lints D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels May 8, 2025
@DaAlbrecht DaAlbrecht force-pushed the 105-camera-modification-fixed-update-lint branch from 3124a84 to 98517bb Compare May 25, 2025 21:38
@DaAlbrecht DaAlbrecht force-pushed the 105-camera-modification-fixed-update-lint branch from 98517bb to a71caa7 Compare May 25, 2025 21:39
@DaAlbrecht DaAlbrecht added D-Complex Quite challenging from either a design or technical perspective. Ask for help! and removed D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels May 25, 2025
@DaAlbrecht DaAlbrecht force-pushed the 105-camera-modification-fixed-update-lint branch from 05736e1 to 512f57e Compare May 27, 2025 20:21
@DaAlbrecht DaAlbrecht marked this pull request as ready for review May 28, 2025 20:46
@DaAlbrecht DaAlbrecht added S-Needs-Review The PR needs to be reviewed before it can be merged and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels May 28, 2025
@BD103 BD103 moved this to This Week in BD103 Work Planning Jun 21, 2025
Comment on lines +109 to +110
if let ExprKind::Path(QPath::Resolved(_, path)) = system_expr.kind
&& let Res::Def(_, def_id) = path.res
Copy link
Member

Choose a reason for hiding this comment

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

For a follow-up: This doesn't handle nested tuples. Meaning I don't think this will lint on:

app.add_systems(FixedUpdate, (unrelated, (also_unrelated, modifies_camera)));
//                                                        ^^^^^^^^^^^^^^^ Won't see this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep! I think I want to solve this first before merging, otherwise it could be confusing.

Copy link
Member

Choose a reason for hiding this comment

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

It is a nursery lint, so you can list it in "Known Issues" if you want this shipped first.

Copy link
Member

Choose a reason for hiding this comment

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

You may be able to use detuple() for this as well! But again, we can definitely push this back to a follow-up.

@DaAlbrecht
Copy link
Collaborator Author

@BD103 Most of the feedback will apply to the other lint as well, since I mostly copied this code from here over ^^. Will update it!

Copy link
Member

@BD103 BD103 left a comment

Choose a reason for hiding this comment

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

There's one quick logic issue and a documentation improvement, then I think I may just merge this and open a few follow-up issues. Thanks for getting this started!

Comment on lines +109 to +110
if let ExprKind::Path(QPath::Resolved(_, path)) = system_expr.kind
&& let Res::Def(_, def_id) = path.res
Copy link
Member

Choose a reason for hiding this comment

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

You may be able to use detuple() for this as well! But again, we can definitely push this back to a follow-up.

@BD103 BD103 enabled auto-merge (squash) June 25, 2025 14:35
@DaAlbrecht
Copy link
Collaborator Author

There's one quick logic issue and a documentation improvement, then I think I may just merge this and open a few follow-up issues. Thanks for getting this started!

Thanks for the fixes and feedback!😊 makes sense im a bit busy with exams so this is a good idea

@BD103 BD103 merged commit bdf9136 into main Jun 25, 2025
10 checks passed
@BD103 BD103 deleted the 105-camera-modification-fixed-update-lint branch June 25, 2025 14:51
@github-project-automation github-project-automation bot moved this from This Week to Done in BD103 Work Planning Jun 25, 2025
@BD103
Copy link
Member

BD103 commented Jun 25, 2025

Thanks for the fixes and feedback!😊 makes sense im a bit busy with exams so this is a good idea

Hey no worries! I had to drop out a bit in the past few months, but I'm happy to take the reins now! (Best of luck studying too :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Related to the linter and custom lints D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Needs-Review The PR needs to be reviewed before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add lint: modifying camera code under FixedUpdate schedule
2 participants