-
Notifications
You must be signed in to change notification settings - Fork 64
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
743 infer legend prop #749
Conversation
132cfa7
to
d5e9616
Compare
d5e9616
to
d7bd0d4
Compare
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.
Great improvement! I left two minor comments. I'd also add it to the CHANGELOG.
cartoframes/viz/legend.py
Outdated
|
||
self._check_prop(_prop) | ||
|
||
print(_prop) |
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.
Remove print
cartoframes/viz/legend.py
Outdated
)) | ||
|
||
def _check_prop(self, _prop): | ||
print(_prop) |
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.
Remove print
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.
As an improvement, I'd move _type
and _prop
initialization to separated methods and I'd leave the responsibility of checking if they're correct to these methods:
def _get_type(self, geom_type):
_type = self._type.get(geom_type)
if isinstance(self._type, dict) and geom_type in self._type
else self._type
self._check_type(self._type)
return _type
def _get_prop(self, _type):
_prop = self._prop if self._prop else self._infer_prop(_type)
self._check_prop(_prop)
return _prop
And then, in get_info
:
def get_info(self, geom_type=None):
# ...
_type = self._get_type(geom_type)
_prop = self._get_prop(_type)
Thanks! Suggestions added. Note: I have updated the changelog of this feature in #741 |
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.
Awesome ✨
Fixes #743