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

Migrate errors to static enums #113

Open
jedel1043 opened this issue Nov 19, 2024 · 3 comments
Open

Migrate errors to static enums #113

jedel1043 opened this issue Nov 19, 2024 · 3 comments
Labels
C-internal Internal library improvements E-easy Easy issue to fix - Good for newcomers E-help wanted Extra attention is needed

Comments

@jedel1043
Copy link
Member

Our current design for TemporalError is essentially a mirror of ECMAScript's error types:

temporal/src/error.rs

Lines 39 to 43 in 345ad54

#[derive(Debug, Clone, PartialEq)]
pub struct TemporalError {
kind: ErrorKind,
msg: Box<str>,
}

This works well for engines that have thousands of errors, with each one having a custom message. However, I don't think we need to do that for temporal_rs, and a more rusty approach will be better suited for this. thiserror sounds ideal for this (moreso because it recently added support for no_std).

@jasonwilliams
Copy link
Member

This works well for engines that have thousands of errors, with each one having a custom message. However, I don't think we need to do that for temporal_rs

I think we do pass through error messages up from the underlying parsers and things, would this design still work with that?

Also, what advantage does thiserror bring over using static enums?

@jedel1043
Copy link
Member Author

jedel1043 commented Nov 20, 2024

I think we do pass through error messages up from the underlying parsers and things, would this design still work with that?

Yep, because the underlying ixdtf uses this design for errors :)

https://docs.rs/ixdtf/latest/ixdtf/enum.ParserError.html

Also, what advantage does thiserror bring over using static enums?

It removes the boilerplate of having to wrap errors generated by inner calls, implement the Error trait and implement the Display trait. Though, we could do it manually if we see that it's not that hard to maintain.

@nekevss
Copy link
Member

nekevss commented Nov 21, 2024

Yeah, the initial design was primarily focused on compatibility with Boa's error type. But it probably is not the most optimal error type for a general purpose library.

@nekevss nekevss added E-easy Easy issue to fix - Good for newcomers E-help wanted Extra attention is needed C-internal Internal library improvements labels Dec 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-internal Internal library improvements E-easy Easy issue to fix - Good for newcomers E-help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants