Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: changes a pylint check in dashboard module #10978

Merged
merged 3 commits into from
Sep 23, 2020
Merged

fix: changes a pylint check in dashboard module #10978

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ee022be
Removed not needed disabled pylint rules from `set_dash_metadata`:
kkucharc Sep 21, 2020
241f6c2
Changed broad exception to KeyError.
kkucharc Sep 21, 2020
618a486
Reverted broad_exception in get_col_type in utils
kkucharc Sep 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion superset/dashboards/dao.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ def bulk_delete(models: Optional[List[Dashboard]], commit: bool = True) -> None:
raise ex

@staticmethod
def set_dash_metadata( # pylint: disable=too-many-locals,too-many-branches,too-many-statements
def set_dash_metadata(
dashboard: Dashboard,
data: Dict[Any, Any],
old_to_new_slice_ids: Optional[Dict[int, int]] = None,
Expand Down
2 changes: 1 addition & 1 deletion superset/databases/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ def get_indexes_metadata(
def get_col_type(col: Dict[Any, Any]) -> str:
try:
dtype = f"{col['type']}"
except Exception: # pylint: disable=broad-except
except KeyError:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think this one may be trickier. I was staring at this and wondering why a KeyError is raised and on the exception handling we are trying to access the same key again with col["type"].__class__.__name__. So the exception KeyError makes sense, because it may be possible, but what we are catching is a serialisation problem, # sqla.types.JSON __str__ has a bug, so using __class__.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dpgaspar for noticing this. Maybe I'm lacking the bigger picture here so excuse me if the question would be silly, but In that case why we are trying to do f"{col['type']}" instead just having dtype = col["type"].__class__.__name__ without try catch?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we want the default to be a call to sqla.type.__str__ and if that fails (on the comment it fails for sqla.types.JSON) then use the class name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, then I'm reverting this change

# sqla.types.JSON __str__ has a bug, so using __class__.
dtype = col["type"].__class__.__name__
return dtype
Expand Down