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

GsonReader: Rethrow exceptions as Gson types #2

Merged
merged 3 commits into from
Mar 7, 2024
Merged

GsonReader: Rethrow exceptions as Gson types #2

merged 3 commits into from
Mar 7, 2024

Conversation

Leo40Git
Copy link
Contributor

Closes #1.

Copy link
Member

@ix0rai ix0rai left a comment

Choose a reason for hiding this comment

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

looks great!
I'd probably pull error handling into a void rethrowJsonExceptions(Runnable) just to cut down on LOC a bit

Don't throw away the original exceptions entirely (they're now used as the cause of the wrapping exceptions)

me: "I should follow ix0rai's advice and extract the exception wrapping code"
also me: makes 5 separate helper functions for various return types
@Leo40Git
Copy link
Contributor Author

looks great! I'd probably pull error handling into a void rethrowJsonExceptions(Runnable) just to cut down on LOC a bit

me: yeah I should do that
also me: makes 5 separate methods for that to cover all the possible return types because Java generics are a lie

Copy link
Contributor

@AlexIIL AlexIIL left a comment

Choose a reason for hiding this comment

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

The performance gains by using per-type functions are unlikely to be worth the added maintenance cost of having 6 separate functions instead of a single one, please instead use DelegateFunction instead. (In addition creating a lambda invoker creates an object too - delegate::beginArray needs to store the delegate in a new object that implements DelegateAction)

For the void variant though you'll need to declare a second type that extends the first, like:

@FunctionalInterface
private interface DelegateAction extends DelegateFunction<Void> {
    @Override
    Void call() throws IOException {
        run();
        return null;
    }

    void run() throws IOException;
}

private void rethrowGsonExceptions(DelegateAction action) throws IOException {
    rethrowGsonExceptions((DelegateFunction<Void>) action);
}

As that way we can keep all the exception handling in one place?
(Alternatively you could call it rethrowGsonExceptionsVoid instead, to avoid the casting to select the right method).

@Leo40Git Leo40Git requested a review from AlexIIL February 4, 2024 17:37
Copy link
Contributor

@AlexIIL AlexIIL left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@TheGlitch76 TheGlitch76 merged commit 7254616 into QuiltMC:trunk Mar 7, 2024
1 check passed
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.

The Gson module should re-throw ParseException as Gson exceptions
5 participants