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

@evaline-ju evaline-ju commented Oct 5, 2020

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 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
Member

@boneskull boneskull left a comment

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(

This comment has been minimized.

@boneskull

boneskull Oct 6, 2020
Member

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

This comment has been minimized.

@evaline-ju

evaline-ju Oct 9, 2020
Author Contributor

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
Copy link
Member

@boneskull boneskull commented Oct 6, 2020

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

@boneskull
Copy link
Member

@boneskull boneskull commented Oct 9, 2020

Thanks!

@boneskull boneskull merged commit fd9fe95 into mochajs:master Oct 9, 2020
20 checks passed
20 checks passed
Retrive head commit message
Details
Check to skip CI
Details
Smoke [Node.js v10 / ubuntu-latest]
Details
Smoke [Node.js v12 / ubuntu-latest]
Details
Smoke [Node.js v14 / ubuntu-latest]
Details
Smoke [Node.js v10 / windows-2019]
Details
Smoke [Node.js v12 / windows-2019]
Details
Smoke [Node.js v14 / windows-2019]
Details
ESLint Check
Details
Markdown Check
Details
Node.js [v10 / ubuntu-latest]
Details
Node.js [v12 / ubuntu-latest]
Details
Node.js [v14 / ubuntu-latest]
Details
Node.js [v10 / windows-2019]
Details
Node.js [v12 / windows-2019]
Details
Node.js [v14 / windows-2019]
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 94.059%
Details
licence/cla Contributor License Agreement is signed.
Details
netlify/mocha/deploy-preview Deploy preview ready!
Details
@boneskull boneskull added this to the v8.2.0 milestone Oct 9, 2020
@evaline-ju evaline-ju deleted the evaline-ju:issue/codes-3656 branch Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.