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

[Enhancement] NFT: deprecate declarative macros. #1042

Merged
merged 14 commits into from
Jul 11, 2023

Conversation

ruseinov
Copy link
Collaborator

@ruseinov ruseinov commented Jun 30, 2023

Quick summary:

  • This PR removes declarative macros that auto-implements NFT traits based on a Contract property implementing them.
  • The result is clear and editable code.
  • This introduces a touch more boilerplate, but allows for customisation and maintains backwards-compatibility.
  • This also bumps package versions, it remains to be seen whether or not this is necessary.

Fixes #422

Continuation: #1041

Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

@ruseinov Nice! Looking at it now, it seems like a trivial change and while it adds the boilerplate, it follows Python Zen "explicit is better than implicit", and I like it. I am thinking that delegation of method calls could be a great thing, but at the same time external crates for trait methods delegation (1, 2) make the code more magical to me. Please, address the comments and I feel we are good to get it merged

CHANGELOG.md Outdated Show resolved Hide resolved
near-sdk-macros/Cargo.toml Outdated Show resolved Hide resolved
examples/non-fungible-token/nft/src/lib.rs Show resolved Hide resolved
ruseinov and others added 3 commits July 4, 2023 10:59
Co-authored-by: Vlad Frolov <frolvlad@gmail.com>
This reverts commit 6fd407c.
@ruseinov ruseinov changed the title [Enhancement] NFT: remove declarative macros. [Enhancement] NFT: deprecate declarative macros. Jul 4, 2023
Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

@ruseinov This PR is self-contained; I am ready to merge it overall. Would you like to submit a separate PR with the docs or would you like to include the docs in this PR?

CHANGELOG.md Outdated
Comment on lines 16 to 18
### Removed
- Removed declarative macros for NFT impl code generation. [PR 1042](https://github.com/near/near-sdk-rs/pull/1042)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's remove duplication

@ruseinov
Copy link
Collaborator Author

ruseinov commented Jul 5, 2023

@ruseinov This PR is self-contained; I am ready to merge it overall. Would you like to submit a separate PR with the docs or would you like to include the docs in this PR?

The docs are WIP, let’s merge when it’s done and cleaned up.

@ruseinov
Copy link
Collaborator Author

ruseinov commented Jul 6, 2023

@frol Fixed the docs, I'm not sure yet how to best tackle more global docs and README.md though, but it's looking much better now.

@ruseinov ruseinov requested a review from frol July 6, 2023 16:10
Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

@ruseinov Overall, I am happy to merge it, but there are some minor discussion points to address.

@agostbiro
Copy link
Contributor

Thanks for implementing this @ruseinov and for the detailed review @frol ! LGTM except for the small remaining comments.

I created #1049 and #1050 as follow up tasks.

@ruseinov
Copy link
Collaborator Author

Thanks for implementing this @ruseinov and for the detailed review @frol ! LGTM except for the small remaining comments.

I created #1049 and #1050 as follow up tasks.

Nice, no problem.

I'm not entirely sure about the links in deprecation notices, could you perhaps make a code suggestion to see how that's supposed to look?

Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

I added fully-qualified paths to the traits in the deprecation notes, so it is at least easier to search on the Internet

@frol frol enabled auto-merge (squash) July 11, 2023 13:38
@frol frol merged commit 7563f4a into near:master Jul 11, 2023
13 checks passed
@ruseinov ruseinov deleted the ru/feature/remove-nft-macros branch July 12, 2023 11:47
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.

Removing boilerplate from token_standards
3 participants