Skip to content

Scatter highlight tests#2

Merged
bclehmann merged 4 commits intobclehmann:scatter-highlightfrom
StendProg:scatter-highlightTests
May 23, 2020
Merged

Scatter highlight tests#2
bclehmann merged 4 commits intobclehmann:scatter-highlightfrom
StendProg:scatter-highlightTests

Conversation

@StendProg
Copy link

Purpose:
Add unit tests for ScatterHighlight math.
Addtionaly replace tabs with spaces back in IHighlightable.cs and PlottableScatter.cs. VS don't show the difference, but github grabs it as changes.

I can't commit to you PR directly, but can make PR to you fork branch.

New functionality (code):
change HighlightIndexes signature from private to protected. This allow test its values in derived test class.

New functionality (image):
Only math of public api IHighligtable was tested.

@StendProg
Copy link
Author

StendProg commented May 23, 2020

There is one point to discuss, i write test for adding nonexisted index to highlight List. And expected function must trow exception.
Current implementation add incorrect index to highlighted List.
After thinking, i decided that this behavior also correct, user can change data array and this index become correct (negative values never become).
So there is test in this PR that check for incorrect indexes and expected that this index added (pass for now).

You have to make a decision that to do with incorrect indexes:

  1. Throw ArgumentOutOfRange
  2. Not throw, but not add this index
  3. Stay as is, and add this index.

@bclehmann bclehmann merged commit 6dc4796 into bclehmann:scatter-highlight May 23, 2020
@bclehmann
Copy link
Owner

I think staying as it is is probably fine, you give the example of the plot being modified, and I think that's a fair reason to not be strict with the user.

bclehmann pushed a commit that referenced this pull request Sep 3, 2021
bclehmann pushed a commit that referenced this pull request Jun 30, 2023
RangePlot plottable and cookbook entry
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants