-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[REF-3228] implement LiteralStringVar and format/retrieval mechanism #3669
[REF-3228] implement LiteralStringVar and format/retrieval mechanism #3669
Conversation
adhami3310
commented
Jul 12, 2024
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.
looks good to the eye.
needs some test cases
Returns: | ||
The hash of the var. | ||
""" | ||
return hash((self._var_name, self._var_type, self._var_data)) |
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 we really need this defined here? my concern is that some subclass adds additional fields, but then doesn't override __hash__
and thus would compare the same as another Var, even though it really was different.
maybe that's not an issue though. i know __hash__
is a bit weird, in that it is used for set
/dict
operations, but then they use equality for collision detection
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.
If it's not defined here we would have to define it elsewhere anyways, so I don't particularly see a way out of it.
Also __eq__
is generally made to use the weird system of comparison to serialize it for the front end instead of actually comparing the two and returning a bool. It feels like this could cause a limitation between true equality and front-end equality.
} | ||
|
||
|
||
def _decode_var_immutable(value: str) -> tuple[ImmutableVarData | None, str]: |
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.
who calls this function? it seems like the functionality is part of LiteralStringVar.create
now and the old-style vars don't need the ImmutableVarData
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's currently being called in __post_init_
of ImmutableVar
. If it's desired we can push the behavior of creating ConcatVarOperation
into there so not use this.
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 think this is good to go