-
Notifications
You must be signed in to change notification settings - Fork 386
Refactor proc macros into v2 #1495
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
base: main
Are you sure you want to change the base?
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
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.
Left a couple of comments
Also, could you explain the idea behind removing all the comments from the generated code?
"DAPP_NAME" | ||
} | ||
|
||
fn version() -> felt252 { | ||
'DAPP_VERSION' | ||
"DAPP_VERSION" |
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 believe the implementation with ByteArray literals instead of felt252 literals won't be able to compile
Can we add a check that all the generated contracts compile without issues?
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.
It won't, but we don't need them to compile here, we just need the macro to generate the correct code. We don't need to add a check for compilation, since the macro is not in charge of that.
Note that using a ByteArray is a temporal solution until the proc macro bug of not accepting shortstrings gets fixed.
'DAPP_NAME' | ||
"DAPP_NAME" | ||
} | ||
fn version() -> felt252 { | ||
'DAPP_VERSION' | ||
"DAPP_VERSION" |
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.
Same issue with literals of a wrong type here
Sadly it is not intentional, but the |
Fixes #1485
PR Checklist