fix: Constants to the confg properties #470
Conversation
This came up in a Gemini review on the backport to 0.3.x
Summary of ChangesHello @kabir, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a code quality improvement by extracting hardcoded configuration property keys into dedicated Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a good step towards improving code maintainability by replacing hardcoded configuration property keys with constants. The changes are applied consistently across several files.
I have a few suggestions to further improve the code by reducing duplication:
- In
AsyncExecutorProducer.java, I suggest making the new constantspublicto allow reuse. - In
MicroProfileConfigProviderTest.java, this would allow removing duplicated constant definitions and an unused import.
This will centralize the constants, reduce duplication, and make future changes easier.
Also, there's a small typo in the pull request title ('confg' instead of 'config').
...le-config/src/test/java/io/a2a/integrations/microprofile/MicroProfileConfigProviderTest.java
Outdated
Show resolved
Hide resolved
| private static final String A2A_EXECUTOR_CORE_POOL_SIZE = "a2a.executor.core-pool-size"; | ||
| private static final String A2A_EXECUTOR_MAX_POOL_SIZE = "a2a.executor.max-pool-size"; | ||
| private static final String A2A_EXECUTOR_KEEP_ALIVE_SECONDS = "a2a.executor.keep-alive-seconds"; |
There was a problem hiding this comment.
These constants are duplicates of those defined in AsyncExecutorProducer. To improve maintainability, you can remove them and instead import the public constants from AsyncExecutorProducer (as suggested in another comment).
After removing these lines, you can add static imports like this at the top of the file:
import static io.a2a.server.util.async.AsyncExecutorProducer.*;There was a problem hiding this comment.
I decided I didn't want to make them public, since the javadoc becomes a bit strange then. It is already written on the fields where this ends up.
| private static final String A2A_EXECUTOR_CORE_POOL_SIZE = "a2a.executor.core-pool-size"; | ||
| private static final String A2A_EXECUTOR_MAX_POOL_SIZE = "a2a.executor.max-pool-size"; | ||
| private static final String A2A_EXECUTOR_KEEP_ALIVE_SECONDS = "a2a.executor.keep-alive-seconds"; |
There was a problem hiding this comment.
These constants are also defined in MicroProfileConfigProviderTest. To avoid duplication, you could change their visibility to public. This would allow them to be reused in the test class, creating a single source of truth for these property keys and improving maintainability.
| private static final String A2A_EXECUTOR_CORE_POOL_SIZE = "a2a.executor.core-pool-size"; | |
| private static final String A2A_EXECUTOR_MAX_POOL_SIZE = "a2a.executor.max-pool-size"; | |
| private static final String A2A_EXECUTOR_KEEP_ALIVE_SECONDS = "a2a.executor.keep-alive-seconds"; | |
| public static final String A2A_EXECUTOR_CORE_POOL_SIZE = "a2a.executor.core-pool-size"; | |
| public static final String A2A_EXECUTOR_MAX_POOL_SIZE = "a2a.executor.max-pool-size"; | |
| public static final String A2A_EXECUTOR_KEEP_ALIVE_SECONDS = "a2a.executor.keep-alive-seconds"; |
This came up in a Gemini review on the backport to 0.3.x This is a follow up for PR a2aproject#468 and issue a2aproject#467
4 participants
This came up in a Gemini review on the backport to 0.3.x
This is a follow up for PR #468 and issue #467