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

Codegen refactoring for operation context params and jmesPath #5219

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cenedhryn
Copy link
Contributor

@cenedhryn cenedhryn commented May 13, 2024

Motivation and Context

Refactors codegen for operation context parameters and jmesPath code generator, and fixes the condition for when to generate the jmesPath runtime.

Modifications

  • The EndpointResolverInterceptorSpec is getting too big. By extracting logic and codegen for Operation Context Params and putting into a separate class, it's easier to read and understand the code
  • Renaming the JmesPath code generator from JmesPathAcceptorGenerator to JmesPathExpressionConverter since Acceptor is a Waiter term. Another name could be JmesPathCodeGenerator
  • During initial development, the conditions for copying the JmesPathRuntime.java.resource was set to true. Now that all endpoint parameter logic is finished, it's updated to checking for the presence of Operation Context Params: If there are waiters, we know there is a runtime. If there are no waiters, we'll generate runtime if there are operation context parameters

Testing

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed
  • I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

@cenedhryn cenedhryn requested a review from a team as a code owner May 13, 2024 22:24
@cenedhryn cenedhryn changed the title [DRAFT] Change name of jmesPath code generator and refactors op context param… [DRAFT] Codegen refactoring for operation context params and jmesPath May 14, 2024
@cenedhryn cenedhryn changed the title [DRAFT] Codegen refactoring for operation context params and jmesPath Codegen refactoring for operation context params and jmesPath May 14, 2024
Copy link

sonarcloud bot commented May 14, 2024

/**
* Knowledge index to get access to operation context parameters.
*/
public final class OperationContextParamsKnowledgeIndex {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: KnowledgeIndex sounds fancy, but I don't know what it means😅. Maybe just OperationContextParamsHelper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not my favorite either, but there is prior art from the SRA implementation and I'd rather not introduce a new term right now. We could rename all the KnowledgeIndex instances in a separate PR.

Copy link
Contributor

@zoewangg zoewangg May 14, 2024

Choose a reason for hiding this comment

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

Ah I see, we have more Helper classes than KnowledgeIndex classes though ;) Anyway, it's codegen class, so it doesn't really matter

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

2 participants