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: small code review fix #14756

Merged
merged 1 commit into from
May 27, 2021
Merged

fix: small code review fix #14756

merged 1 commit into from
May 27, 2021

Conversation

cccs-jc
Copy link
Contributor

@cccs-jc cccs-jc commented May 21, 2021

just fixing typing

@codecov
Copy link

codecov bot commented May 21, 2021 •
edited
Loading

Codecov Report

Merging #14756 (502dfae) into master (590fe20) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #14756   +/-   ##
=======================================
  Coverage   77.52%   77.52%           
=======================================
  Files         960      960           
  Lines       48819    48819           
  Branches     6120     6120           
=======================================
  Hits        37847    37847           
  Misses      10769    10769           
  Partials      203      203
Flag Coverage Δ
hive 81.12% <100.00%> (ø)
mysql 81.39% <100.00%> (ø)
postgres 81.42% <100.00%> (ø)
presto 81.11% <100.00%> (ø)
python 81.95% <100.00%> (ø)
sqlite 81.03% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/jinja_context.py 89.30% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 590fe20...502dfae. Read the comment docs.

etr2460
etr2460 reviewed May 21, 2021
View reviewed changes
superset/jinja_context.py
@@ -297,7 +297,7 @@ def get_filters(self, column: str, remove_filter: bool = False) -> List[Filter]:
filters: List[Filter] = []

for flt in form_data.get("adhoc_filters", []):
val: Union[str, List[str]] = flt.get("comparator")
val: Union[Any, List[Any]] = flt.get("comparator")
Copy link
Member

@etr2460 etr2460 May 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

does this need to be Any? Maybe there's a more precise type we can use here. What values are expected for the comparator?

Copy link
Member

Choose a reason for hiding this comment

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

If we want to be fully explicit, I believe the full type would be Union[bool, str, int, float, List[bool], List[str], List[int], List[float]], but that feels slightly verbose. Could potentially be shortened by introducing an alias FilterValue = bool, str, int, float and then doing Union[FilterValue, List[FilterValue]], but even that feels kinda stuffy. However, I'd personally vote for keeping it Any for now.

villebro
villebro approved these changes May 26, 2021
View reviewed changes
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM - I think we can follow up with more explicit types later when we get around to properly introducing TypedDict.

superset/jinja_context.py
@@ -297,7 +297,7 @@ def get_filters(self, column: str, remove_filter: bool = False) -> List[Filter]:
filters: List[Filter] = []

for flt in form_data.get("adhoc_filters", []):
val: Union[str, List[str]] = flt.get("comparator")
val: Union[Any, List[Any]] = flt.get("comparator")
Copy link
Member

Choose a reason for hiding this comment

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

If we want to be fully explicit, I believe the full type would be Union[bool, str, int, float, List[bool], List[str], List[int], List[float]], but that feels slightly verbose. Could potentially be shortened by introducing an alias FilterValue = bool, str, int, float and then doing Union[FilterValue, List[FilterValue]], but even that feels kinda stuffy. However, I'd personally vote for keeping it Any for now.

@villebro villebro merged commit c519efe into apache:master May 27, 2021
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
@cccs-jc
small code review fix (apache#14756)
c95dc18 
Co-authored-by: cccs-jc <cccs-jc@cyber.gc.ca>
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
@cccs-jc
small code review fix (apache#14756)
d1cb6c7 
Co-authored-by: cccs-jc <cccs-jc@cyber.gc.ca>
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@cccs-jc
small code review fix (apache#14756)
8881711 
Co-authored-by: cccs-jc <cccs-jc@cyber.gc.ca>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etr2460 etr2460 etr2460 left review comments

@villebro villebro villebro approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS 🚢 1.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@cccs-jc @etr2460 @villebro @mistercrunch