Skip to content

Conversation

@Piyushrathoree
Copy link

Resolves #8375

Changes: added unit test for src/color/color_spaces/hsb.js

PR Checklist

@Piyushrathoree
Copy link
Author

@davepagurek please take look

@davepagurek
Copy link
Contributor

Hey @limzykenneth, being more familiar with the HSB converters, how does this look to you?

Copy link
Member

@limzykenneth limzykenneth left a comment

Choose a reason for hiding this comment

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

One inline thing but the rest are good. Thanks.

Comment on lines 18 to 25
// Helper function to compare arrays with tolerance
const arrayApproximately = (arr1, arr2, delta = 0.01) => {
if (arr1.length !== arr2.length) return false;
for (let i = 0; i < arr1.length; i++) {
if (Math.abs(arr1[i] - arr2[i]) > delta) return false;
}
return true;
};
Copy link
Member

Choose a reason for hiding this comment

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

This part is not being used and is not defined in the right way for an assertion. If you'd want to continue exploring this you can follow the implementation here: https://github.com/processing/p5.js/blob/dev-2.0/test/js/chai_helpers.js#L7-L12.

Copy link
Author

Choose a reason for hiding this comment

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

thanks , implemented.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that it should be directly attached to assert like this, especially in this file. Also it is still not being used, if it is not to be used then it don't really need to be there.

@Piyushrathoree
Copy link
Author

@limzykenneth removed that . check the tests as well.

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.

3 participants