Skip to content
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

Change serializer errors to use error codes #4464

Merged
merged 2 commits into from
Oct 9, 2020

Conversation

evaline-ju
Copy link
Contributor

Description of the Change

After the implementations of #3467 and #3666, there are a few instances in the code that do not use error codes yet. This PR addresses the instances in the serializer code and does not cover all remaining instances of lack of error codes (I mainly wanted to make sure I was on the right track here before trying to change as many remaining errors as possible).

Alternate Designs

N/A

Why should this be in core?

Using codes is a best practice like in Node.js, codes help identify errors as mocha errors at a glance

Benefits

Using codes is a best practice like in Node.js, codes help identify errors as mocha errors at a glance

Possible Drawbacks

None I'm aware of

Applicable issues

Contributes to #3656, semver-patch

@coveralls
Copy link

coveralls commented Oct 5, 2020

Coverage Status

Coverage increased (+0.1%) to 94.059% when pulling 14b116b on evaline-ju:issue/codes-3656 into 8f5d6a9 on mochajs:master.

Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM, but had a comment

@@ -131,7 +134,12 @@ class SerializableEvent {
*/
constructor(eventName, originalValue, originalError) {
if (!eventName) {
throw new Error('expected a non-empty `eventName` string argument');
throw createInvalidArgumentValueError(
Copy link
Contributor

Choose a reason for hiding this comment

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

if eventName is undefined (e.g., someone wrote new SerializableEvent()) then one could argue this is an invalid type (it should be a string; not undefined) instead of an invalid value. contrast that with passing '' for eventName--the type is correct but the value is invalid. in this case, we're checking for falsy, which is both a type and a value (because JavaScript)--and is intended--so I don't feel strongly that we need two different errors here. just wanted to mention it

further, these two types of errors are unlikely to be caused by an end-user--it's an error only Mocha contributors should ever encounter. we don't usually do this sort of exhaustive type-checking unless it's user input

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me. I wasn't tied to the two different error types, so I've gone ahead and changed this one to throw a type error too.

@boneskull boneskull added area: node.js command-line-or-Node.js-specific area: parallel mode Regarding parallel mode semver-minor implementation requires increase of "minor" version number; "features" area: usability concerning user experience or interface labels Oct 6, 2020
@boneskull
Copy link
Contributor

calling this semver-minor as it does not fix any known bug, but is rather additive.

@boneskull
Copy link
Contributor

Thanks!

@boneskull boneskull merged commit fd9fe95 into mochajs:master Oct 9, 2020
@boneskull boneskull added this to the v8.2.0 milestone Oct 9, 2020
@evaline-ju evaline-ju deleted the issue/codes-3656 branch October 9, 2020 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: node.js command-line-or-Node.js-specific area: parallel mode Regarding parallel mode area: usability concerning user experience or interface semver-minor implementation requires increase of "minor" version number; "features"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants