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

Error handling experiment #5429

Merged
merged 33 commits into from
Apr 26, 2021
Merged

Error handling experiment #5429

merged 33 commits into from
Apr 26, 2021

Conversation

nf-s
Copy link
Contributor

@nf-s nf-s commented Apr 15, 2021

First pass at improving error handling (mostly in Terria.ts)

Related to #3486

This will need changes in TerriaMap - TerriaJS/TerriaMap#526

Checklist

  • There are unit tests to verify my changes are correct or unit tests aren't applicable (if so, write quick reason why unit tests don't exist)
  • I've updated CHANGES.md with what I changed.

@steve9164 steve9164 self-requested a review April 19, 2021 09:05
@KeyboardSounds
Copy link
Contributor

+10 TerriaBucks for writing docs 🌟

*/
export default class Result<T> {
/** Convenience function to return a Result with an error */
static error(error: TerriaErrorOptions | TerriaError): Result<undefined> {
Copy link
Member

Choose a reason for hiding this comment

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

Interesting choice to require the T type to include undefined (so that Result.error is assignable). I haven't thought through all of the implications of that yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

T doesn't have to include undefined. It just is undefined if you use the Result.error convenience constructor thing

Comment on lines 106 to 107
private readonly value: T,
private readonly error?: TerriaError
Copy link
Member

Choose a reason for hiding this comment

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

I'd like there to be some way to differentiate between a recoverable error and a non-recoverable error, unless you think that shouldn't be one of the aims of the Result type. Is that expressed outside the definition of the Result class using undefined as a value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that could totally be apart of the Result or TerriaError class - I just left it out as I didn't find any need in the "initial loading" stuff in Terria.ts.

I figured we could add it wherever when it's needed

lib/Core/TerriaError.ts Outdated Show resolved Hide resolved
lib/Core/TerriaError.ts Outdated Show resolved Hide resolved
@@ -825,48 +842,74 @@ export default class Terria {
}
);

const errors: TerriaError[] = [];
Copy link
Member

Choose a reason for hiding this comment

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

Bookmark

nf-s and others added 3 commits April 21, 2021 20:19
Co-authored-by: Emma Krantz <3796838+KeyboardSounds@users.noreply.github.com>
@nf-s
Copy link
Contributor Author

nf-s commented Apr 21, 2021

I applied this to Traits.fromJson() in a new PR - #5440 - just to see how well it would go

@nf-s nf-s marked this pull request as ready for review April 22, 2021 04:29
@steve9164
Copy link
Member

steve9164 commented Apr 22, 2021

Just waiting on the build error test fail to merge.

CHANGES.md Show resolved Hide resolved
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.

None yet

3 participants