-
Notifications
You must be signed in to change notification settings - Fork 252
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
Conversation
There was a problem hiding this 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
Co-authored-by: Vlad Frolov <[email protected]>
This reverts commit 6fd407c.
This reverts commit 5a4cd6b.
There was a problem hiding this 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
### Removed | ||
- Removed declarative macros for NFT impl code generation. [PR 1042](https://github.com/near/near-sdk-rs/pull/1042) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove duplication
The docs are WIP, let’s merge when it’s done and cleaned up. |
@frol Fixed the docs, I'm not sure yet how to best tackle more global docs and |
There was a problem hiding this 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.
near-contract-standards/src/non_fungible_token/enumeration/mod.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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
Quick summary:
Fixes #422
Continuation: #1041