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

fix: macro sanity remove importing types #571

Merged
merged 10 commits into from
Nov 18, 2021
Merged

Conversation

austinabell
Copy link
Contributor

@austinabell austinabell commented Sep 16, 2021

Closes #570. This still has the issue that it requires importing the traits in the decl macro, but those are harder to untangle without breaking change (although technically this is a breaking change) Actually, 404d6ec (#571) achieves what I'm thinking of, but not certain this is the cleanest way to keep the macro sanity.

@austinabell
Copy link
Contributor Author

Okay, so testing depends on the traits moved strictly in the scope of the macro in 404d6ec. This means that we have a few options:

  1. Leave traits imported through declarative macro
  • could be a collision if someone expects to pull into a module, but there shouldn't be a need to do this if the macro is being used
  1. Add a prelude module in nft standards with all NFT traits so that devs can easily pull them into scope?
  • This seems like a negative because it requires knowing about prelude conventions and might be a blocker for non-rust devs
  • Alternative being having to manually bring in each trait specifically (painful since can't easily add ones used in macros)

@ChaoticTempest
Copy link
Member

From what I've seen, in macro_rules! there's no sanity when it comes to use so people have just directly reference the whole path when it comes time to use it.

Okay, so testing depends on the traits moved strictly in the scope of the macro in 404d6ec. [...]

Let's just directly reference the whole path then:

    impl $crate::non_fungible_token::core::NonFungibleTokenCore for $contract {

@austinabell
Copy link
Contributor Author

From what I've seen, in macro_rules! there's no sanity when it comes to use so people have just directly reference the whole path when it comes time to use it.

Okay, so testing depends on the traits moved strictly in the scope of the macro in 404d6ec. [...]

Let's just directly reference the whole path then:

    impl $crate::non_fungible_token::core::NonFungibleTokenCore for $contract {

Can't do this, because you need the trait in scope to call the inner implementation. If this, then it requires devs to import all NFT traits, which will break all existing uses. This is what I suggested in mitigating with point 2 above.

@ChaoticTempest
Copy link
Member

Can't do this, because you need the trait in scope to call the inner implementation. If this, then it requires devs to import all NFT traits, which will break all existing uses. This is what I suggested in mitigating with point 2 above.

ahh I see what you mean. Breaking macro changes are fun aren't they /s.

hmm, how about we still go with point 1, but instead also have the macro take multiple (contract, token) pairs so we'd only be able to invoke these macros once per module? That way we force the user to not specify them multiple times

@austinabell austinabell merged commit 149883f into master Nov 18, 2021
@austinabell austinabell deleted the austin/nft_s/macro branch November 18, 2021 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Token standards add types to file scope
2 participants