New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Error handling #4
Conversation
src/index.js
Outdated
*/ | ||
command(cmd) { | ||
if (!this.commands[cmd]) { | ||
throw new Error(`Specified command: ${cmd} is invalid`); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
src/index.js
Outdated
@@ -147,6 +154,10 @@ export default class Luxafor { | |||
* @return {object} Instance | |||
*/ | |||
color(r = 0, g = 0, b = 0) { | |||
if (invalidNums([...arguments])) { | |||
throw new Error(`Specified values: ${[...arguments]} are invalid`); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should move this to exec() method, so it's not duplicated for each additional method.
src/index.js
Outdated
}); | ||
|
||
return invalid; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we're not using the returned value, it might make sense to send back a bool
function hasInvalidArguments(nums) {
return nums.some(num =>
!Number.isInteger(num) || num < 0 || num > 255;
);
}
], | ||
] | ||
`; | ||
exports[`Luxafor exec Should execute the write for only one led position 1`] = `Array []`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is expected for this test. Should it have written to the entire lux instead?
it('Should execute the write for only one led position', () => {
const instance = setup()
.color(0, 255, 0)
.exec();
expect(
instance.device.data
).toMatchSnapshot();
});
@@ -79,6 +90,38 @@ export default class Luxafor { | |||
return timingBytes[command]; | |||
} | |||
|
|||
validate() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how much I like how this is done.
@@ -92,6 +135,8 @@ export default class Luxafor { | |||
* TODO: Should pattern clear out command byte? | |||
*/ | |||
write({command = 'color', position = 'both', r = 0, g = 0, b = 0, pattern, speed, repeat}) { | |||
this.validate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, not sure how much I like this here.
@@ -109,30 +154,24 @@ export default class Luxafor { | |||
* @returns {object} Instance | |||
*/ | |||
exec() { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a bug, not being able to do commands without specifying any ledPositions.
Added basic error handling. Also change ledPositions to always be an array.