Skip to content

Lint: missing_reflect check if all fields implement Reflect otherwise Applicability ::MaybeIncorrect#389

Merged
BD103 merged 11 commits intomainfrom
141-fix-missing-reflect
Apr 25, 2025
Merged

Lint: missing_reflect check if all fields implement Reflect otherwise Applicability ::MaybeIncorrect#389
BD103 merged 11 commits intomainfrom
141-fix-missing-reflect

Conversation

@DaAlbrecht
Copy link
Copy Markdown
Collaborator

fixes #141

@DaAlbrecht DaAlbrecht force-pushed the 141-fix-missing-reflect branch from c861397 to 3af04c8 Compare April 23, 2025 11:24
@DaAlbrecht DaAlbrecht added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Apr 23, 2025
@BD103 BD103 added A-Linter Related to the linter and custom lints C-Usability An improvement that makes the API more pleasant labels Apr 23, 2025
@BD103
Copy link
Copy Markdown
Member

BD103 commented Apr 23, 2025

Hah, you beat me to it! I'll review this once you mark it as ready :)

@DaAlbrecht DaAlbrecht force-pushed the 141-fix-missing-reflect branch from 3af04c8 to 5626fdc Compare April 23, 2025 13:15
@DaAlbrecht
Copy link
Copy Markdown
Collaborator Author

Hah, you beat me to it! I'll review this once you mark it as ready :)

haha I'm sorry (; long train ride back and got bored ^^ but I'm out of Battery now :(

@DaAlbrecht DaAlbrecht marked this pull request as ready for review April 23, 2025 19:25
@DaAlbrecht DaAlbrecht requested a review from BD103 April 23, 2025 19:25
@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 Apr 23, 2025
@BD103 BD103 mentioned this pull request Apr 24, 2025
3 tasks
Copy link
Copy Markdown
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.

The concept is great, it just skips over enums :)

I made some suggestions that should cover them, but let me know if you disagree with anything or have any questions!

@DaAlbrecht
Copy link
Copy Markdown
Collaborator Author

The concept is great, it just skips over enums :)

I made some suggestions that should cover them, but let me know if you disagree with anything or have any questions!

omg, you are right I completely missed that ^^

@DaAlbrecht DaAlbrecht requested a review from BD103 April 25, 2025 08:23
Copy link
Copy Markdown
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.

Awesome!

Comment on lines +151 to +155
.variants
.iter()
.flat_map(|variant| variant.data.fields())
.copied()
.collect(),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Much cleaner than my for loop, good thinking!

@BD103 BD103 merged commit 326722d into main Apr 25, 2025
10 checks passed
@BD103 BD103 deleted the 141-fix-missing-reflect branch April 25, 2025 22:22
BD103 added a commit that referenced this pull request Apr 30, 2025
This PR prepares `bevy_lint` for v0.3.0, following the [release
guide](https://github.com/TheBevyFlock/bevy_cli/blob/e85154b9bd5851a0dcac3815853ec8194d362fb3/bevy_lint/docs/how-to/release.md).
Once this is marked ready for review, this will need at least one
approval from a Bevy maintainer and no outstanding concerns. Once this
is merged, a feature freeze will be enacted until the [post-release PR
is
merged](https://github.com/TheBevyFlock/bevy_cli/blob/e85154b9bd5851a0dcac3815853ec8194d362fb3/bevy_lint/docs/how-to/release.md#post-release).

## Blockers

- [x] #380
- [x] #389
- [x] #391

## For Reviewers

Now is your chance to look through the
[docs](https://thebevyflock.github.io/bevy_cli/bevy_lint/index.html),
the changelog, and the migration guide to ensure everything is in
working order. If you wish to test the linter, you may install it from
this branch:

```sh
rustup toolchain install nightly-2025-04-03 \
    --component rustc-dev \
    --component llvm-tools-preview

rustup run nightly-2025-04-03 cargo install \
    --git https://github.com/TheBevyFlock/bevy_cli.git \
    --branch linter-v0.3.0 \
    --locked \
    bevy_lint
```

You're encouraged to run the linter on several projects to ensure it
works correctly. Some good places to start include:

- [The Engine](https://github.com/bevyengine/bevy)
- [`bevy_new_2d`](https://github.com/TheBevyFlock/bevy_new_2d)
- [The Official
Template](https://github.com/bevyengine/bevy_github_ci_template)

Thank you for all of your help!

## Next Steps

- Release on Github
- Announce the release on Discord, Mastodon, and other social medias
- Potentially publish an announcement [blog
post](https://bd103.github.io/)
- TheBevyFlock/bevy_new_2d#365

---------

Co-authored-by: DAA <42379074+DaAlbrecht@users.noreply.github.com>
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 C-Usability An improvement that makes the API more pleasant 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.

Lint missing_reflect does not check for fields that cannot be reflected

2 participants