Skip to content

Conversation

@elbertronnie
Copy link
Contributor

@elbertronnie elbertronnie commented Nov 25, 2022

Objective

Solution

  • Replaced all the types with their fully quallified names
  • Replaced all trait methods and inherent methods on dyn traits with their fully qualified names
  • Made a new file fq_std.rs that contains structs corresponding to commonly used Structs and Traits from std. These structs are replaced by their respective fully qualified names when used inside quote!

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Reflection Runtime information about types C-Code-Quality A section of code that is hard to understand or change labels Nov 25, 2022
Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

LGTM! Just one non-blocking comment

@MrGVSV
Copy link
Member

MrGVSV commented Nov 25, 2022

bors try

bors bot added a commit that referenced this pull request Nov 25, 2022
@bors
Copy link
Contributor

bors bot commented Nov 25, 2022

try

Build failed:

@elbertronnie elbertronnie requested a review from MrGVSV December 3, 2022 11:24
@@ -0,0 +1,45 @@
use proc_macro2::TokenStream;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Not super necessary but it would be nice to either have a module-level doc here stating the purpose of these FQ*** types or doc comments on the individual structs (or both haha).

It's pretty simple/self-explanatory so definitely not required, but I like to make sure the barrier to contributing is low for newcomers :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added the doc too.

@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 Dec 3, 2022
use proc_macro2::TokenStream;
use quote::{quote, ToTokens};

// This module contains unit structs that should be used inside `quote!` and `spanned_quote!` using the variable interpolation syntax in place of their equivalent structs and traits present in `std`.
Copy link
Member

Choose a reason for hiding this comment

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

Normally module doc comments are made using //! at the very top of the file (above imports). If we include these, could we move them to the top to be consistent with that format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have moved them to the top and used //! for the docs.
Initially, I had thought that using //! would make this module visible in the user-facing documentation of Bevy but later realized that won't happen.

@alice-i-cecile alice-i-cecile 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 4, 2022
@alice-i-cecile
Copy link
Member

Blocking on moving the module comment, then I'll merge.

@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 Dec 4, 2022
@cart
Copy link
Member

cart commented Dec 5, 2022

bors r+

bors bot pushed a commit that referenced this pull request Dec 5, 2022
# Objective

- Fixes #3004 

## Solution

- Replaced all the types with their fully quallified names
- Replaced all trait methods and inherent methods on dyn traits with their fully qualified names
- Made a new file `fq_std.rs` that contains structs corresponding to commonly used Structs and Traits from `std`. These structs are replaced by their respective fully qualified names when used inside `quote!`
@bors bors bot changed the title Make proc macros hygienic in bevy_reflect_derive [Merged by Bors] - Make proc macros hygienic in bevy_reflect_derive Dec 5, 2022
@bors bors bot closed this Dec 5, 2022
@james7132 james7132 added this to the 0.10 milestone Dec 6, 2022
ickshonpe pushed a commit to ickshonpe/bevy that referenced this pull request Dec 6, 2022
# Objective

- Fixes bevyengine#3004 

## Solution

- Replaced all the types with their fully quallified names
- Replaced all trait methods and inherent methods on dyn traits with their fully qualified names
- Made a new file `fq_std.rs` that contains structs corresponding to commonly used Structs and Traits from `std`. These structs are replaced by their respective fully qualified names when used inside `quote!`
@elbertronnie elbertronnie deleted the fix-unhygienic-macros branch January 17, 2023 12:01
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
# Objective

- Fixes bevyengine#3004 

## Solution

- Replaced all the types with their fully quallified names
- Replaced all trait methods and inherent methods on dyn traits with their fully qualified names
- Made a new file `fq_std.rs` that contains structs corresponding to commonly used Structs and Traits from `std`. These structs are replaced by their respective fully qualified names when used inside `quote!`
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- Fixes bevyengine#3004 

## Solution

- Replaced all the types with their fully quallified names
- Replaced all trait methods and inherent methods on dyn traits with their fully qualified names
- Made a new file `fq_std.rs` that contains structs corresponding to commonly used Structs and Traits from `std`. These structs are replaced by their respective fully qualified names when used inside `quote!`
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 C-Bug An unexpected or incorrect behavior 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

Status: Done

Development

Successfully merging this pull request may close these issues.

Reflect proc-macro causes compile errors when using custom Result type

5 participants