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

refactor: Error constructors do not need new #7481

Merged
merged 1 commit into from
Apr 27, 2023
Merged

Conversation

erights
Copy link
Member

@erights erights commented Apr 22, 2023

See endojs/endo#1562

A step towards turning on more @jessie-check directives. See #3876 and #5497

Jessie prohibits new. One trivial case of revising code to conform is to remove the new on calls to the *Error( constructors. For these specifically, their construct behavior is the same as their call behavior. This also gets rid of a bit of visual noise, make code a bit more readable.

Note to reviewers: I isolated this step by itself because

  • I did this in a purely mechanical manner. At least so far, as of this first commit, before responding to any review comments
  • IIUC, it should be clearly correctness preserving, without requiring much understanding of the code being refactored.

@erights
Copy link
Member Author

erights commented Apr 22, 2023

Aha! As discovered from looking at CI errors, not all *Errors mentioned by our sources are builtin errors, or adequately like builtin errors. Specifically, CommanderError is a JS subclass of Error and InvalidArgumentError is a JS subclass of CommanderError. These are both apparently from https://github.com/tj/commander.js/ , which is used by both endo cli and agoric-sdk agoric-cli .

@erights erights changed the title refactor: Error constructor does not need new refactor: Error constructor does not need new Apr 22, 2023
@erights erights changed the title refactor: Error constructor does not need new refactor: Error constructors do not need new Apr 22, 2023
@erights erights requested a review from michaelfig April 22, 2023 22:26
@@ -359,7 +359,7 @@ async function replay(transcriptFile) {
}
syscallResults = {};
if (divergent && !argv.ignoreConcurrentWorkerDivergences) {
throw new Error('Divergent execution between workers');
throw Error('Divergent execution between workers');
Copy link
Member

Choose a reason for hiding this comment

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

Why do this in files that aren't marked @jessy-check? What keeps them from coming back?

Copy link
Member Author

Choose a reason for hiding this comment

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

For general stylistic consistency, and as a step towards being able to add @jessie-check to more files. Even for the files we aren't yet able to mark @jessie-check, we want as much stylistic consistency as practical with those we do.

Copy link
Member

Choose a reason for hiding this comment

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

I don't disagree, but it's not clear to me how other reviewers will learn this new part of house style. I don't see what keeps the new operators from re-appearing since, evidently, folks are used to putting them there.

Perhaps add a few reviewers to get a critical mass? In particular, kernel maintainers.

Or discuss it internally?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've been doing Error(...) instead of new Error(...) for as long as I can remember.

Copy link
Contributor

@FUDCo FUDCo left a comment

Choose a reason for hiding this comment

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

This makes me happy.

@erights erights added the automerge:rebase Automatically rebase updates, then merge label Apr 27, 2023
@mergify mergify bot merged commit a0a427e into master Apr 27, 2023
@mergify mergify bot deleted the markm-errors-sans-new branch April 27, 2023 03:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants