-
Notifications
You must be signed in to change notification settings - Fork 249
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] Deprecate Fungible Token declarative macros. #1054
Conversation
pub use self::core::FungibleTokenCore; | ||
pub use crate::storage_management::StorageManagement; | ||
pub use core_impl::FungibleToken; | ||
pub use macros::*; | ||
pub use resolver::FungibleTokenResolver; |
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 align on one way of importing things. I like self::
, but it is not the style used before, so let's not use it now.
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.
Agreed.
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.
There is a reason I used self
there and it has to do with the fact that otherwise we get an ambiguity error. I have changed that to use the fully qualified path crate::fungible_token
..
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.
Note, that we do use self
for non_fungible_token
imports. For that same reason - there is a name ambiguity problem with core
.
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 We should consider migrating all the imports to use self::
or crate::
for local imports, but it is a separate discussion.
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.
self
is interesting for disambiguation scenarios, because the path ends up being shorter.
This seems to be an a good shape for another round of reviews. |
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.
Overall, looks good to me. See the last couple of comments from me.
@ruseinov The only remaining item left is the CI. Add the offending packages to the ignore list and create a separate issue to address it in the future. |
Closes #1049