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: remove unneeded ErrorKinds #3936

Merged
merged 28 commits into from
Feb 21, 2020

Conversation

bartlomieju
Copy link
Member

@bartlomieju bartlomieju commented Feb 9, 2020

Follow up to #3662

  • replace DenoError.Other with Error
  • replace DenoError.UrlParse, DenoError.InvalidPath and DenoError.ImportMissingPrefix with URIError
  • replace DenoError.Http with HttpError (?) - the same error that fetch throws
  • replace following with IoError (?)
    • NotFound
    • PermissionDenied
    • ConnectionRefused
    • ConnectionReset
    • ConnectionAborted
    • NotConnected
    • AddrInUse
    • AddrNotAvailable
    • BrokenPipe
    • AlreadyExists
    • WouldBlock - shouldn't be used removed
    • InvalidInput - TypeError
    • InvalidData - TypeError (possible just Error?)
    • TimedOut
    • Interrupted - shouldn't be used
    • WriteZero
    • UnexpectedEof
    • BadResource
  • replace InvalidSeekMode with TypeError
  • replace TooLarge with Error
  • replace UnixError with SystemError (?) -> explode that to ERRNO, map ERRNO to Deno defined errors, if one does not exist then panic
  • remove Diagnostic and JSError

@bartlomieju bartlomieju changed the title use Error and URIError refactor: remove DenoError and ErrorKind Feb 9, 2020
@bartlomieju bartlomieju changed the title refactor: remove DenoError and ErrorKind refactor: remove unneeded ErrorKinds Feb 19, 2020
@bartlomieju bartlomieju requested a review from ry February 19, 2020 21:35
cli/deno_error.rs Outdated Show resolved Hide resolved
cli/deno_error.rs Outdated Show resolved Hide resolved
cli/deno_error.rs Outdated Show resolved Hide resolved
cli/deno_error.rs Outdated Show resolved Hide resolved
cli/deno_error.rs Outdated Show resolved Hide resolved
@@ -252,6 +247,8 @@ impl GetErrorKind for DlopenError {
}
}

// NOTE(bartlomieju): seems this is necessary - can't use ErrBox here
// TODO(bartlomieju): ultimately this should be rewritten to `RuntimeError`?
impl GetErrorKind for dyn AnyError {
fn kind(&self) -> ErrorKind {
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 think this impl is necessary. I suggest:

fn kind(&self) -> ErrorKind {
  unreachable!()
}

Copy link
Member Author

@bartlomieju bartlomieju Feb 19, 2020

Choose a reason for hiding this comment

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

Not really, it seems necessary, tried to remove that and doesn't work. Need to figure this out

cli/js/errors.ts Outdated Show resolved Hide resolved
cli/deno_error.rs Outdated Show resolved Hide resolved
cli/deno_error.rs Outdated Show resolved Hide resolved
@bartlomieju
Copy link
Member Author

@ry I'd prefer to land this PR as is if CI goes green. We should be able to rewrite the rest tomorrow.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

Removing approval since work is still in progress. Needs a final review.

@bartlomieju bartlomieju requested a review from ry February 21, 2020 05:37
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

I have some comments - but I guess they're addressed in your next patch.

@bartlomieju bartlomieju merged commit dd8a109 into denoland:master Feb 21, 2020
@bartlomieju bartlomieju deleted the error_refactor branch February 21, 2020 15:36
@ry ry mentioned this pull request Feb 21, 2020
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 21, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Feb 1, 2021
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.

2 participants