Skip to content

Add regression test / fix for DataLabelTool#804

Merged
aaronayres35 merged 5 commits into
masterfrom
fix/550-AbstractPlotRenderers-return-shapes
Jul 7, 2021
Merged

Add regression test / fix for DataLabelTool#804
aaronayres35 merged 5 commits into
masterfrom
fix/550-AbstractPlotRenderers-return-shapes

Conversation

@aaronayres35
Copy link
Copy Markdown
Contributor

@aaronayres35 aaronayres35 commented Jun 28, 2021

This PR adds a regression test and a fix for the specific error reported on #550.

550 is a bit broader in scope though and would also include #802 (and probably more) to be closed.
Note the changes here were pulled from #726 in an effort to make smaller more reviewable PRs. However, there is some interconnectedness between these PRs. This PR should not be merged first, as it will break things without the change in #802

As the issue mentions, currently the DataLabelTool is forced to assume the BasaeXYPlot way or the Base2DPlot way.
"This difference forces objects that interact with AbstractPlotRenderers to make an assumption about the behavior of the map_screen method. For example, the DataLabelTool assumes the map_screen return shape matches that of BaseXYPlot for a single point ((2,)) ... And therefore this tool throws an exception when used with a Base2DPlot"

This PR makes DataLabelTool follow the Base2DPlot way (which we are pushing as the new standard), and #802 makes BaseXYPlot do the same. On this branch, DataLabelTool will not work with a BaseXYPlot.

EDIT: I've added a failing test for use of DataLabelToolwith a BaseXYPlot. This test will pass once #802 is merged

@aaronayres35
Copy link
Copy Markdown
Contributor Author

#802 was merged, so I merged master with this branch and now CI passes as expected.

@aaronayres35 aaronayres35 requested a review from rahulporuri July 7, 2021 13:52
Copy link
Copy Markdown
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

LGTM

@aaronayres35 aaronayres35 merged commit 7a72bdf into master Jul 7, 2021
@aaronayres35 aaronayres35 deleted the fix/550-AbstractPlotRenderers-return-shapes branch July 7, 2021 13:57
@corranwebster corranwebster added the needs backport Needs to be backported to current maint/* label Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs backport Needs to be backported to current maint/*

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants