Fix #396, display primitive doesn't need BPM or illum mask.

Open MichaelRFairhurst opened this issue 2 years ago • 1 comments

It looked like #396 was an easy fix waiting to be picked up. Hoping this PR is an easy review and helpful.

Let me know if it seems better to use a mocking library; I didn't see any in use in the project and worked without it.

Also worth nothing: I did use TDD on this, that is to say, I know that this new test fails without the corresponding update to Visualize.display. However, the test in this design could stop being effective if parameter defaults are changed/methods are renamed/etc. If there's a better approach to testing this, I would be happy to change the test accordingly. It also may not be an important regression to keep as a test, if CI speed is an issue.

Cheers!

Mar 04 '23 05:03 MichaelRFairhurst

Thanks Michael! We just noticed the PR.

Our in-house Jenkins test server cannot run on the PR probably because the changes are in your fork. We don't get external PRs very often. We're looking into defining a procedure.

The changes look good to me and we'll take them, once we got the Jenkins thing sorted.

Thanks for your contribution.

Apr 01 '25 18:04 KathleenLabrie