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: Errors do not need "new" #2445

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

erights
Copy link
Contributor

@erights erights commented Sep 4, 2024

Closes: #XXXX
Refs: Agoric/agoric-sdk#10030

Description

Errors do not need new

For all construction calls to a *Error constructor, the new is not needed. IOW, just calling the *Error constructor as a function has the same effect.

Security Considerations

none

Scaling Considerations

none

Documentation Considerations

none

Testing Considerations

none

Compatibility Considerations

none

Upgrade Considerations

none

@erights erights self-assigned this Sep 4, 2024
@erights erights marked this pull request as ready for review September 4, 2024 20:27
@erights erights requested a review from kriskowal September 4, 2024 20:28
@erights erights force-pushed the markm-errors-no-new branch from 7e45a4d to e0fb0a1 Compare September 4, 2024 21:22
@erights erights requested a review from turadg September 5, 2024 04:57
mergify bot pushed a commit to Agoric/agoric-sdk that referenced this pull request Sep 5, 2024
Closes: #XXXX
Refs: endojs/endo#2445

## Description

Errors do not need `new`

For all construction calls to a `*Error` constructor, the `new` is not needed. IOW, just calling the `*Error` constructor as a function has the same effect.

### Security Considerations

none
### Scaling Considerations

none
### Documentation Considerations

none
### Testing Considerations

none
### Compatibility Considerations

none
### Upgrade Considerations

none
@erights erights force-pushed the markm-errors-no-new branch from e0fb0a1 to a6852c9 Compare September 7, 2024 20:48
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

If this change is worth making, it's worth making durably.

Requesting that the lint rules be configured to error on new Error:

{
  "rules": {
    "no-restricted-syntax": [
      "error",
      {
        "selector": "NewExpression[callee.name='Error']",
        "message": "Use Error(msg) instead of new Error(msg)"
      }
    ]
  }
}

@mhofman
Copy link
Contributor

mhofman commented Sep 24, 2024

Requesting that the lint rules be configured to error on new Error:

Should this handle other Error constructors, maybe with a RegExp attribute selector?

@gibson042
Copy link
Contributor

Requesting that the lint rules be configured to error on new Error:

Should this handle other Error constructors, maybe with a RegExp attribute selector?

Yes!

{
  "rules": {
    "no-restricted-syntax": [
      "error",
      {
        "selector": "NewExpression[callee.name=/^([A-Z]\\w*)?Error$/]",
        "message": "Use Error(msg) instead of new Error(msg)"
      }
    ]
  }
}

@erights erights force-pushed the markm-errors-no-new branch from a6852c9 to 0899aec Compare October 14, 2024 20:14
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.

👍 assuming ESLint is configured to enforce this (either in this PR or a different one).

@erights erights force-pushed the markm-errors-no-new branch from 0899aec to 5727d9c Compare October 29, 2024 00:11
@erights erights force-pushed the markm-errors-no-new branch from 5727d9c to 028de23 Compare November 17, 2024 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants