improve arguments error detection for inspector #1492
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chancancode
reviewed
Nov 14, 2023
| @@ -101,9 +102,11 @@ export class EnvironmentImpl implements Environment { | |||
| // Delegate methods and values | |||
| public isInteractive: boolean; | |||
|
|
|||
| isArgumentError: typeof isArgumentError; | |||
There was a problem hiding this comment.
I know I made up the original name, but since we are now stashing it on a more general object, we probably want to name it something slightly more specific and less likely to collide/confuse with other meaning, like isArgumentCaptureError, didErrorDuringCapture or something similar.
chancancode
reviewed
Nov 14, 2023
| const ARGUMENT_ERROR = Symbol("ARGUMENT_ERROR"); | ||
|
|
||
| export function isArgumentError(arg: unknown): arg is ArgumentError { | ||
| return arg !== null && typeof arg === "object" && arg[ARGUMENT_ERROR]; |
There was a problem hiding this comment.
Suggested change
| return arg !== null && typeof arg === "object" && arg[ARGUMENT_ERROR]; | |
| return arg !== null && typeof arg === "object" && (arg as { [ARGUMENT_ERROR]?: unknown })[ARGUMENT_ERROR]`; |
...I think
chancancode
reviewed
Nov 14, 2023
| return arg !== null && typeof arg === "object" && arg[ARGUMENT_ERROR]; | ||
| } | ||
|
|
||
| export class ArgumentErrorImpl { |
There was a problem hiding this comment.
Shouldn't need to be exported, yeah?
chancancode
approved these changes
Nov 14, 2023
There was a problem hiding this comment.
Other than fixing the lint/type stuff this broadly looks good to me, I don't have a strong opinion on whether we merge it now or after the error branch. I'll leave that up to @NullVoxPopuli
patricklx
force-pushed
the
patch-2
branch
2 times, most recently
from
8517768 to
4e9db94
Compare
patricklx
commented
Nov 14, 2023
| } | ||
|
|
||
|
|
||
| export declare function isArgumentError(arg: unknown): arg is ArgumentError |
|
@NullVoxPopuli can we have another look here? |
NullVoxPopuli
approved these changes
Feb 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.