-
-
Notifications
You must be signed in to change notification settings - Fork 790
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
feat[ux]: support constants from imported interface #4253
base: master
Are you sure you want to change the base?
Conversation
@@ -10,7 +10,7 @@ | |||
StructureException, | |||
UnfoldableNode, | |||
) | |||
from vyper.semantics.analysis.base import Modifiability | |||
from vyper.semantics.analysis.base import Modifiability, VarInfo |
Check notice
Code scanning / CodeQL
Cyclic import Note
vyper.semantics.analysis.base
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4253 +/- ##
==========================================
- Coverage 91.35% 88.89% -2.47%
==========================================
Files 109 109
Lines 15636 15681 +45
Branches 3436 3457 +21
==========================================
- Hits 14285 13940 -345
- Misses 921 1228 +307
- Partials 430 513 +83
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
vyper/semantics/types/module.py
Outdated
constants = [ | ||
(node.target.id, node.target._metadata["varinfo"]) | ||
for node in module_t.variable_decls | ||
if node.target._metadata["varinfo"].modifiability is Modifiability.CONSTANT |
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.
can just check node.is_constant
vyper/semantics/types/module.py
Outdated
) -> None: | ||
validate_unique_method_ids(list(functions.values())) | ||
|
||
members = functions | events | structs | ||
constants = constants if constants else {} |
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.
constants or {}
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.
tbh we should do this for all the other things too, since in most cases we end up passing an empty dict for most of these parameters
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.
do you mean to set the default for these params to None? should I do it in this PR?
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.
Yes and yes!
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 am thinking that since _from_lists
is already passing a dictionary (either empty or with values) for all the params, and it is the only caller of the ctor for InterfaceT
, maybe we can require a dict instead for all the same params in the ctor for InterfaceT
?
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 made the changes in 69e5b3e to show what I meant in the previous comment.
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.
thanks! can we also add flags into scope? #4121
@@ -21,7 +21,7 @@ | |||
from vyper.semantics.types.base import TYPE_T, VyperType, is_type_t | |||
from vyper.semantics.types.function import ContractFunctionT | |||
from vyper.semantics.types.primitives import AddressT | |||
from vyper.semantics.types.user import EventT, StructT, _UserType | |||
from vyper.semantics.types.user import EventT, FlagT, StructT, _UserType |
Check notice
Code scanning / CodeQL
Cyclic import Note
What I did
Resolves #4176
How I did it
How to verify it
Commit message
Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)
Description for the changelog
Cute Animal Picture