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

Use serde elision index #4712

Merged
merged 2 commits into from
May 9, 2023
Merged

Use serde elision index #4712

merged 2 commits into from
May 9, 2023

Conversation

srchase
Copy link
Contributor

@srchase srchase commented May 9, 2023

Issue

Issue number, if available, prefixed with "#"

Description

This PR updates the Shape visitors and protocol generators to use the SerdeElision KnowledgeIndex that was converted in smithy-lang/smithy-typescript#759

It also adds the UnaryFunctionCall class that was originally added to smithy-typescript (and which is removed in smithy-lang/smithy-typescript#760).

Testing

Ran yarn generate-clients to assure there were no generated code differences.

Additionally, ran ./gradlew clean build from the codegen directory to assure that the java.lang.OutOfMemoryError: Java heap space errors were not thrown as described in #4692.

Additional context

Add any other context about the PR here.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@srchase srchase requested a review from a team as a code owner May 9, 2023 18:36
@srchase srchase force-pushed the use-serde-elision-index branch from 5f80e2d to 6b81152 Compare May 9, 2023 18:37
Comment on lines 117 to 119
if (shape.getId().getName().equals("DenseBooleanMap")) {
System.out.println("here");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

😅 Let's remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops. Removed.

/**
* For handling expressions that may be unary function calls.
*/
public abstract class UnaryFunctionCall {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this class abstract?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

there might've been a linter warning when a class only has static methods and isn't abstract

@@ -55,16 +55,19 @@ final class JsonShapeSerVisitor extends DocumentShapeSerVisitor {

private final BiFunction<MemberShape, String, String> memberNameStrategy;

JsonShapeSerVisitor(GenerationContext context) {
JsonShapeSerVisitor(GenerationContext context, boolean serdeElisionEnabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

interface change is ok, right? if any external consumers upgrade, they change their calls at the same time?

/**
* For handling expressions that may be unary function calls.
*/
class UnaryFunctionCall {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this repo and smithy-ts have different static analysis requirements. It might've needed to be abstract there when there were no instance level methods or properties. That check made sense to me.

@srchase srchase merged commit 5d01f58 into aws:main May 9, 2023
@srchase srchase deleted the use-serde-elision-index branch May 9, 2023 20:51
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants