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

Moved fq_std from bevy_reflect_derive to bevy_macro_utils #9956

Merged
merged 3 commits into from
Oct 2, 2023

Conversation

bushrat011899
Copy link
Contributor

@bushrat011899 bushrat011899 commented Sep 29, 2023

Objective

Solution

Moved fq_std from bevy_reflect_derive to bevy_macro_utils. This does make the FQ* types public where they were previously private, which is a change to the public-facing API, but I don't believe a breaking one. Additionally, I've done a basic QA pass over the bevy_macro_utils crate, adding deny(unsafe), warn(missing_docs), and documentation where required.

@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change A-Reflection Runtime information about types A-Utils Utility functions and types labels Sep 29, 2023
@MrGVSV MrGVSV 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 Sep 29, 2023
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

No objections on an implementation level, but it'd be nice to keep the code quality up while where at it.

crates/bevy_reflect/bevy_reflect_derive/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_macro_utils/src/lib.rs Show resolved Hide resolved
Added missing documentation, added `warn(missing_docs)`, and `deny(unsafe)` as standard best-practice.
@bushrat011899
Copy link
Contributor Author

I've made the changes requested by @james7132 which increase the overall quality of bevy_macro_utils. I also took the liberty of adding deny(unsafe) (since it wasn't using unsafe anyway), and splitting out the contents of lib.rs into bevy_manifest.rs and label.rs to allow lib.rs to be exclusively used for declaring modules and publishing public members. None of this has materially affected the PR beyond adding documentation and general cleaning.

Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

This goes above and beyond what I asked for. Thanks!

@james7132 james7132 added this pull request to the merge queue Oct 2, 2023
Merged via the queue into bevyengine:main with commit e5dbde8 Oct 2, 2023
24 of 25 checks passed
ameknite pushed a commit to ameknite/bevy that referenced this pull request Oct 3, 2023
…engine#9956)

# Objective

- Fixes bevyengine#9363

## Solution

Moved `fq_std` from `bevy_reflect_derive` to `bevy_macro_utils`. This
does make the `FQ*` types public where they were previously private,
which is a change to the public-facing API, but I don't believe a
breaking one. Additionally, I've done a basic QA pass over the
`bevy_macro_utils` crate, adding `deny(unsafe)`, `warn(missing_docs)`,
and documentation where required.
ameknite pushed a commit to ameknite/bevy that referenced this pull request Oct 3, 2023
…engine#9956)

# Objective

- Fixes bevyengine#9363

## Solution

Moved `fq_std` from `bevy_reflect_derive` to `bevy_macro_utils`. This
does make the `FQ*` types public where they were previously private,
which is a change to the public-facing API, but I don't believe a
breaking one. Additionally, I've done a basic QA pass over the
`bevy_macro_utils` crate, adding `deny(unsafe)`, `warn(missing_docs)`,
and documentation where required.
regnarock pushed a commit to regnarock/bevy that referenced this pull request Oct 13, 2023
…engine#9956)

# Objective

- Fixes bevyengine#9363

## Solution

Moved `fq_std` from `bevy_reflect_derive` to `bevy_macro_utils`. This
does make the `FQ*` types public where they were previously private,
which is a change to the public-facing API, but I don't believe a
breaking one. Additionally, I've done a basic QA pass over the
`bevy_macro_utils` crate, adding `deny(unsafe)`, `warn(missing_docs)`,
and documentation where required.
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
…engine#9956)

# Objective

- Fixes bevyengine#9363

## Solution

Moved `fq_std` from `bevy_reflect_derive` to `bevy_macro_utils`. This
does make the `FQ*` types public where they were previously private,
which is a change to the public-facing API, but I don't believe a
breaking one. Additionally, I've done a basic QA pass over the
`bevy_macro_utils` crate, adding `deny(unsafe)`, `warn(missing_docs)`,
and documentation where required.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types A-Utils Utility functions and types C-Code-Quality A section of code that is hard to understand or change 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.

fq_std module should be moved to bevy_macro_utils
4 participants