-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
[viz] Adding get_df typing #8034
[viz] Adding get_df typing #8034
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.
1 question. as a side note, i'm surprised that the typechecker still passes now that it knows you're trying to get a param on an optional value. Is that the desired behavior?
superset/viz.py
Outdated
@@ -181,7 +182,7 @@ def get_samples(self): | |||
df = self.get_df(query_obj) | |||
return df.to_dict(orient="records") | |||
|
|||
def get_df(self, query_obj=None): | |||
def get_df(self, query_obj: Dict[str, Any] = None) -> Optional[pd.DataFrame]: |
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.
Shouldn't the query_obj
be typed as an Optional[Dict[str, Any]]
?
superset/viz.py
Outdated
@@ -725,7 +726,7 @@ class MarkupViz(BaseViz): | |||
def query_obj(self): | |||
return None | |||
|
|||
def get_df(self, query_obj=None): | |||
def get_df(self, query_obj: Dict[str, Any] = None) -> Optional[pd.DataFrame]: |
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 comment here as above
@etr2460 I addressed your comments. |
fc6e21e
to
030adb1
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.
lgtm with one more typing nit to fix
@@ -1854,7 +1859,7 @@ class IFrameViz(BaseViz): | |||
def query_obj(self): | |||
return None | |||
|
|||
def get_df(self, query_obj=None): | |||
def get_df(self, query_obj: Dict[str, Any] = None) -> Optional[pd.DataFrame]: |
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.
missed an optional here
I don't feel strongly either way, i.e. returning an empty |
@john-bodley are you planning on getting this in? |
da3b9ad
to
51a983e
Compare
CATEGORY
Choose one
SUMMARY
I came across a bug in
viz.py
whereget_csv
which uses logic like,i.e., it assumes that
df
is of typepd.DataFrame
whereas it can optionally beNone
. Going forward I'm not sure what the correct logic should be but I've added basic type hints to theget_df(...)
method to inform developers that it can be either apd.DataFrame
orNone
. Note this PR does not fix the bug.Note my preference would be for
get_df(...)
to only return apd.DataFrame
, that way we wouldn't need to write logic likeif df is None
in numerous places. One question is ifdf
isNone
should the return value fromget_csv()
beNone
or should it be a serialized emptypd.DataFrame
?TEST PLAN
CI.
ADDITIONAL INFORMATION
REVIEWERS
to: @etr2460 @mistercrunch @villebro