HouseKeeping: Moved errors to one place #201

ajaymathur · 2019-10-24T06:03:31Z

Moved ExitError and ValidationError out from cli and config package respectively and updated the usage.

changeset-bot · 2019-10-24T06:03:33Z

🦋 Changeset is good to go

Latest commit: 1cdb85c

We got this.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

…eir usage across repository packages

codecov · 2019-10-24T06:08:32Z

Codecov Report

❗ No coverage uploaded for pull request base (master@51a0d76). Click here to learn what that means.
The diff coverage is 60%.

@@            Coverage Diff            @@
##             master     #201   +/-   ##
=========================================
  Coverage          ?   76.35%           
=========================================
  Files             ?       41           
  Lines             ?     1049           
  Branches          ?      228           
=========================================
  Hits              ?      801           
  Misses            ?      244           
  Partials          ?        4

Impacted Files	Coverage Δ
packages/cli/src/commands/publish/index.ts	`82.35% <ø> (ø)`
packages/cli/src/commands/publish/npm-utils.ts	`6.77% <ø> (ø)`
packages/config/src/index.ts	`100% <100%> (ø)`
packages/errors/src/index.ts	`50% <50%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51a0d76...1cdb85c. Read the comment docs.

emmatown · 2019-10-24T06:42:48Z

packages/errors/src/index.ts

+}
+
+export class ExitError extends Error {
+  constructor(public code: number) {


TIL about this syntax, thanks!

emmatown · 2019-10-24T06:43:52Z

packages/errors/src/index.ts

+
+export class ValidationError extends Error {
+  constructor(message: string) {
+    super(`Validation Error: ${message}`);


I'd rather have it pass the message directly because either one of two things will happen with the error, the error will be handled specially and some code can make it say that it's a validation error or the error will be logged and the name of error will be shown.

Suggested change

super(`Validation Error: ${message}`);

super(message);

Hey @mitchellhamilton,

If I follow your recommendation correctly it is because if we catch the error and log it, We don't want say it is validation error right?

Example:

try { throw new ValidationError('Something something') } catch (err) { /** * Will logging `Validation Error: Something something` be useful or `Something * something` **/ console.log(err); }

I just realized that extending Error doesn't show the right name but we can use extendable-error to fix that and by doing that, when the error is logged, it'll look like this which includes the text ValidationError:

🦋 error { ValidationError: Something something 🦋 error at /Users/mitchell/projects/changesets/packages/cli/src/index.ts:61:9 🦋 error at Object.<anonymous> (/Users/mitchell/projects/changesets/packages/cli/src/index.ts:60:1) 🦋 error at Module._compile (internal/modules/cjs/loader.js:778:30) 🦋 error at Module._compile (/Users/mitchell/projects/changesets/node_modules/pirates/lib/index.js:99:24) 🦋 error at Module._extensions..js (internal/modules/cjs/loader.js:789:10) 🦋 error at Object.newLoader (/Users/mitchell/projects/changesets/node_modules/pirates/lib/index.js:104:7) 🦋 error at Module.load (internal/modules/cjs/loader.js:653:32) 🦋 error at tryModuleLoad (internal/modules/cjs/loader.js:593:12) 🦋 error name: 'ValidationError', 🦋 error _error: 🦋 error Error: 🦋 error at new ExtendableError (/Users/mitchell/projects/changesets/node_modules/extendable-error/bld/C:/Projects/extendable-error/src/index.ts:14:19) 🦋 error at new ValidationError (/Users/mitchell/projects/changesets/packages/cli/src/index.ts:56:5) 🦋 error at /Users/mitchell/projects/changesets/packages/cli/src/index.ts:61:9 🦋 error at Object.<anonymous> (/Users/mitchell/projects/changesets/packages/cli/src/index.ts:60:1) 🦋 error at Module._compile (internal/modules/cjs/loader.js:778:30) 🦋 error at Module._compile (/Users/mitchell/projects/changesets/node_modules/pirates/lib/index.js:99:24) 🦋 error at Module._extensions..js (internal/modules/cjs/loader.js:789:10) 🦋 error at Object.newLoader (/Users/mitchell/projects/changesets/node_modules/pirates/lib/index.js:104:7) 🦋 error at Module.load (internal/modules/cjs/loader.js:653:32) 🦋 error at tryModuleLoad (internal/modules/cjs/loader.js:593:12) }

Also, now that I'm thinking about it, you can remove the constructor as well since inheriting ExtendableError's constructor will be fine here.

…or better message logging

ajaymathur · 2019-10-24T21:45:17Z

Strange the tests for failing in CI but pass in local.

emmatown

Thanks!!

ajaymathur · 2019-10-24T23:21:16Z

packages/errors/src/index.ts

@@ -1,15 +1,19 @@
 import ExtendableError from "extendable-error";

 export class GitError extends ExtendableError {
-  constructor(public code: number, message: string) {
+  code: number;
+  constructor(code: number, message: string) {


Wondering why did this happen. We can look at it later. 🙂

ajaymathur added 2 commits October 24, 2019 17:05

added ExitError and ValidationError to errors package and replaced th…

68b063d

…eir usage across repository packages

added changeset

8872a0d

ajaymathur force-pushed the chore/move-errors-to-one-place branch from ab9b5c1 to 8872a0d Compare October 24, 2019 06:06

ajaymathur changed the title ~~Moved errors to one place~~ HouseKeeping: Moved errors to one place Oct 24, 2019

ajaymathur added the repository maintenance label Oct 24, 2019

emmatown reviewed Oct 24, 2019

View reviewed changes

ajaymathur added 2 commits October 25, 2019 08:26

extend extendableError package instead of extending JS Error class, f…

0287142

…or better message logging

updated changesets

46c83cf

emmatown added 2 commits October 25, 2019 08:15

Maybe it's the cache?

0aa57ed

Try a thing

1cdb85c

emmatown approved these changes Oct 24, 2019

View reviewed changes

emmatown merged commit 5ababa0 into master Oct 24, 2019

emmatown deleted the chore/move-errors-to-one-place branch October 24, 2019 22:32

github-actions bot mentioned this pull request Oct 24, 2019

Version Packages #199

Merged

ajaymathur commented Oct 24, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HouseKeeping: Moved errors to one place #201

HouseKeeping: Moved errors to one place #201

ajaymathur commented Oct 24, 2019

changeset-bot bot commented Oct 24, 2019 •

edited

codecov bot commented Oct 24, 2019 •

edited

emmatown Oct 24, 2019

emmatown Oct 24, 2019

ajaymathur Oct 24, 2019

emmatown Oct 24, 2019 •

edited

ajaymathur commented Oct 24, 2019

emmatown left a comment

ajaymathur Oct 24, 2019

HouseKeeping: Moved errors to one place #201

HouseKeeping: Moved errors to one place #201

Conversation

ajaymathur commented Oct 24, 2019

changeset-bot bot commented Oct 24, 2019 • edited

🦋 Changeset is good to go

codecov bot commented Oct 24, 2019 • edited

Codecov Report

emmatown Oct 24, 2019

Choose a reason for hiding this comment

emmatown Oct 24, 2019

Choose a reason for hiding this comment

ajaymathur Oct 24, 2019

Choose a reason for hiding this comment

emmatown Oct 24, 2019 • edited

Choose a reason for hiding this comment

ajaymathur commented Oct 24, 2019

emmatown left a comment

Choose a reason for hiding this comment

ajaymathur Oct 24, 2019

Choose a reason for hiding this comment

changeset-bot bot commented Oct 24, 2019 •

edited

codecov bot commented Oct 24, 2019 •

edited

emmatown Oct 24, 2019 •

edited