-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add more descriptive pydeck types classes #4897
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,101 @@ | ||
import abc | ||
import json | ||
from collections.abc import Iterable | ||
|
||
TYPE_IDENTIFIER = "@@type" | ||
FUNCTION_IDENTIFIER = "@@=" | ||
ENUM_IDENTIFIER = "@@#" | ||
|
||
|
||
class Type(metaclass=abc.ABCMeta): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would suggest naming this something more descriptive than "Type", which seems a little generic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since it's a base class, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Naming base classes with
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right. This base class isn't public so I'm not too concerned with what the name should be, and welcome those suggestions. I'd also consider prefixing with |
||
"""Base class for Pydeck defined type""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
def __init__(self, value): | ||
self._value = value | ||
|
||
def __repr__(self): | ||
return self.type_name + "(" + str(self.json()) + ")" | ||
|
||
@property | ||
@abc.abstractmethod | ||
def type_name(self): | ||
"""Name of Type; used in default __repr__""" | ||
|
||
@abc.abstractmethod | ||
def json(self): | ||
"""JSON representation of object for serialization""" | ||
|
||
|
||
class Enum(Type): | ||
"""Enum type | ||
|
||
Interpret as an Enum, resolving in the JSON configuration | ||
""" | ||
|
||
type_name = "Enum" | ||
|
||
def __init__(self, value): | ||
if not isinstance(value, str): | ||
raise TypeError("Enum value must be a string") | ||
|
||
super(Enum, self).__init__(value) | ||
|
||
def json(self): | ||
return ENUM_IDENTIFIER + self._value | ||
|
||
|
||
class Function(Type): | ||
"""Function type | ||
|
||
Interpret as a function, parsing unquoted character strings as identifiers | ||
""" | ||
|
||
type_name = "Function" | ||
|
||
def json(self): | ||
if isinstance(self._value, Iterable) and not isinstance(self._value, str): | ||
return FUNCTION_IDENTIFIER + "[{}]".format(", ".join(map(str, self._value))) | ||
|
||
return FUNCTION_IDENTIFIER + str(self._value) | ||
|
||
|
||
class Identifier(Type): | ||
"""Identifier type | ||
""" | ||
|
||
type_name = "Identifier" | ||
|
||
def __init__(self, value): | ||
if not isinstance(value, str): | ||
raise TypeError("Identifier value must be a string") | ||
|
||
super(Identifier, self).__init__(value) | ||
|
||
def __str__(self): | ||
return str(self._value) | ||
|
||
def json(self): | ||
return self._value | ||
|
||
|
||
class Literal(Type): | ||
"""A Literal object | ||
|
||
Can be any JSON-encodable object | ||
""" | ||
|
||
type_name = "Literal" | ||
|
||
def __init__(self, value): | ||
try: | ||
json.dumps(value) | ||
except TypeError as e: | ||
raise TypeError("Literal value must be JSON serializable\n" + e) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're going to get a TypeError in your TypeError message, since the string and error don't concatenate. Maybe
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes you're right. I think I'm looking for |
||
|
||
super(Literal, self).__init__(value) | ||
|
||
def __str__(self): | ||
return '"{}"'.format(self._value) | ||
|
||
def json(self): | ||
return self._value |
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.
This will also fail for
"a" + b + "c"
(results ina" + b + "c
)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 the question here is what is the desired behavior? Is
"a" + b + "c"
->a + b + c
more desired?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 would expect
"a" + b + "c"
to be"a" + b + "c"
to match the behavior of"a" + "c" + b
. The type result changing so dramatically from changing the order of operators is surprising.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.
Right I'd also prefer that but it's a breaking change.