fix: Partial revert of 17236 #17383
Merged
fix: Partial revert of 17236 #17383
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
john-bodley
requested review from
etr2460,
villebro and
zhaoyongjie
and removed request for
etr2460
john-bodley
changed the title
john-bodley
force-pushed
the
john-bodley--partial-revert-17236
branch
from
634e5f2
to
96cdacc
Compare
john-bodley
changed the title
john-bodley
force-pushed
the
john-bodley--partial-revert-17236
branch
from
96cdacc
to
5681933
Compare
Codecov Report
@@ Coverage Diff @@
## master #17383 +/- ##
=======================================
Coverage 77.02% 77.02%
=======================================
Files 1038 1038
Lines 56013 56016 +3
Branches 7735 7735
=======================================
+ Hits 43142 43145 +3
Misses 12613 12613
Partials 258 258
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
etr2460
approved these changes
Nov 11, 2021
| @@ -1326,6 +1326,11 @@ def get_metric_names(metrics: Sequence[Metric]) -> List[str]: | |||
| return [metric for metric in map(get_metric_name, metrics) if metric] | |||
|
|
|||
|
|
|||
| def get_first_metric_name(metrics: Sequence[Metric]) -> Optional[str]: | |||
There was a problem hiding this comment.
this should probably be get_first_metric_label, but i'll stamp because it's only a revert
There was a problem hiding this comment.
Although @etr2460 the function call get_metric_names but assigns these to the metric_labels variable thus the variable as opposed to the function could be misnamed.
AAfghahi
pushed a commit
that referenced
this pull request
Jan 10, 2022
Co-authored-by: John Bodley <john.bodley@airbnb.com>
mistercrunch
added
🏷️ bot
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
This is a partial revert of #17236 as it seems other chart types: all ECharts, Bar Chart, etc. already perform an implicit ordering if undefined, i.e., the ship has sailed on this one—previously I wasn't aware just how prevalent the behavior was and thus the fallback seems like the rule rather than the exception. I also added updated comments to reflect this behavior.
Personally I'm a tad upset/frustrated that this behavior differs by chart type (search for
sort_byin viz.py) as this simply adds confusion to the user and leads to a very inconsistent UX. When we introduce new behaviors we should:UPDATING.mdso institutions can inform/educate users of said changes.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
CI.
ADDITIONAL INFORMATION