Code cleanups#1180
Merged
Merged
Conversation
Reviewer's GuideBroad codebase cleanup and modernization: extracted shared health/CLI parsing logic, centralized repeated validation and health checks, modernized Java idioms (switch expressions, String/collection helpers, URL handling), simplified method signatures and assertions, and fixed documentation tables/paths. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In CapabilitiesCleanup.printSimpleTable, replacing
separator.append("-".repeat(widths[i]))withseparator.repeat("-", widths[i])introduces a compile error because StringBuilder has norepeatmethod; this should be reverted or implemented viaappend("-".repeat(widths[i]))or a small helper. - The pattern-matching switch in MarkdownRenderer.visit(CustomNode) (e.g.,
switch (customNode) { case TableHead tableHead -> ... }) relies on newer Java pattern-matching semantics; please confirm the project’s target JDK supports this or fall back toinstanceofchecks to avoid compatibility issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In CapabilitiesCleanup.printSimpleTable, replacing `separator.append("-".repeat(widths[i]))` with `separator.repeat("-", widths[i])` introduces a compile error because StringBuilder has no `repeat` method; this should be reverted or implemented via `append("-".repeat(widths[i]))` or a small helper.
- The pattern-matching switch in MarkdownRenderer.visit(CustomNode) (e.g., `switch (customNode) { case TableHead tableHead -> ... }`) relies on newer Java pattern-matching semantics; please confirm the project’s target JDK supports this or fall back to `instanceof` checks to avoid compatibility issues.
## Individual Comments
### Comment 1
<location path="apps/wanaku-cli/src/main/java/ai/wanaku/cli/main/commands/capabilities/CapabilitiesCleanup.java" line_range="197" />
<code_context>
for (int i = 0; i < widths.length; i++) {
if (i > 0) separator.append("-+-");
- separator.append("-".repeat(widths[i]));
+ separator.repeat("-", widths[i]);
}
System.out.println(separator);
</code_context>
<issue_to_address>
**issue (bug_risk):** StringBuilder has no repeat method, causing a compilation failure here.
`StringBuilder`/`StringBuffer` don’t define `repeat`, so this change won’t compile. Use `separator.append("-".repeat(widths[i]));` as before, or add a small helper to append a character `widths[i]` times.
</issue_to_address>
### Comment 2
<location path="capabilities-quarkus-sdk/wanaku-capabilities-base/src/main/java/ai/wanaku/core/capabilities/common/CapabilityChecks.java" line_range="11-16" />
<code_context>
+ * Creates a new response build for checking the grpc port
+ * @param grpcServer The gRPC server instance
+ * @param name the check name
+ * @return a new HealtCheckResponse
+ */
+ static HealthCheckResponse grpcPortCheck(GrpcServer grpcServer, String name) {
</code_context>
<issue_to_address>
**nitpick (typo):** Minor typo and wording issue in the Javadoc for grpcPortCheck.
The Javadoc currently says `Creates a new response build` and `HealtCheckResponse`. Please update the description to something like `Creates a new health check response for the gRPC server port` and fix the return tag to `@return a new HealthCheckResponse` for clarity and correctness.
```suggestion
/**
* Creates a new health check response for the gRPC server port
* @param grpcServer The gRPC server instance
* @param name the check name
* @return a new HealthCheckResponse
*/
```
</issue_to_address>
### Comment 3
<location path="apps/wanaku-operator/deploy/helm/wanaku-operator/README.md" line_range="16-25" />
<code_context>
+| `app.livenessProbe.httpGet.scheme` | The http schema to use for the probe. | HTTP |
</code_context>
<issue_to_address>
**issue (typo):** Use "HTTP scheme" instead of "http schema" in the probe descriptions.
For `app.livenessProbe.httpGet.scheme`, `app.readinessProbe.httpGet.scheme`, and `app.startupProbe.httpGet.scheme`, “schema” is incorrect in this HTTP context. Please update the text to refer to the “HTTP scheme” instead (e.g., “The HTTP scheme to use for the probe.”).
Suggested implementation:
```
| `app.livenessProbe.httpGet.scheme` | The HTTP scheme to use for the probe. | HTTP |
```
```
| `app.readinessProbe.httpGet.scheme` | The HTTP scheme to use for the probe. | HTTP |
```
The same wording likely exists for `app.startupProbe.httpGet.scheme` elsewhere in this README (or a related values/README file). Apply the same replacement there:
- Change “The http schema to use for the probe.” to “The HTTP scheme to use for the probe.”
If there are other occurrences of “http schema” in this file or related chart docs, they should also be updated for consistency.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+11
to
+16
| /** | ||
| * Creates a new response build for checking the grpc port | ||
| * @param grpcServer The gRPC server instance | ||
| * @param name the check name | ||
| * @return a new HealtCheckResponse | ||
| */ |
There was a problem hiding this comment.
nitpick (typo): Minor typo and wording issue in the Javadoc for grpcPortCheck.
The Javadoc currently says Creates a new response build and HealtCheckResponse. Please update the description to something like Creates a new health check response for the gRPC server port and fix the return tag to @return a new HealthCheckResponse for clarity and correctness.
Suggested change
| /** | |
| * Creates a new response build for checking the grpc port | |
| * @param grpcServer The gRPC server instance | |
| * @param name the check name | |
| * @return a new HealtCheckResponse | |
| */ | |
| /** | |
| * Creates a new health check response for the gRPC server port | |
| * @param grpcServer The gRPC server instance | |
| * @param name the check name | |
| * @return a new HealthCheckResponse | |
| */ |
Comment on lines
+16
to
+25
| | `app.livenessProbe.httpGet.scheme` | The http schema to use for the probe. | HTTP | | ||
| | `app.livenessProbe.initialDelaySeconds` | The amount of time to wait before starting to probe. | 5 | | ||
| | `app.livenessProbe.periodSeconds` | The period in which the action should be called. | 10 | | ||
| | `app.livenessProbe.successThreshold` | The success threshold to use. | 1 | | ||
| | `app.livenessProbe.timeoutSeconds` | The amount of time to wait for each action. | 10 | | ||
| | `app.ports.http` | The http port to use for the probe. | 8081 | | ||
| | `app.readinessProbe.failureThreshold` | The failure threshold to use. | 3 | | ||
| | `app.readinessProbe.httpGet.path` | The http path to use for the probe. | /q/health/ready | | ||
| | `app.readinessProbe.httpGet.scheme` | The http schema to use for the probe. | HTTP | | ||
| | `app.readinessProbe.initialDelaySeconds` | The amount of time to wait before starting to probe. | 5 | |
There was a problem hiding this comment.
issue (typo): Use "HTTP scheme" instead of "http schema" in the probe descriptions.
For app.livenessProbe.httpGet.scheme, app.readinessProbe.httpGet.scheme, and app.startupProbe.httpGet.scheme, “schema” is incorrect in this HTTP context. Please update the text to refer to the “HTTP scheme” instead (e.g., “The HTTP scheme to use for the probe.”).
Suggested implementation:
| `app.livenessProbe.httpGet.scheme` | The HTTP scheme to use for the probe. | HTTP |
| `app.readinessProbe.httpGet.scheme` | The HTTP scheme to use for the probe. | HTTP |
The same wording likely exists for app.startupProbe.httpGet.scheme elsewhere in this README (or a related values/README file). Apply the same replacement there:
- Change “The http schema to use for the probe.” to “The HTTP scheme to use for the probe.”
If there are other occurrences of “http schema” in this file or related chart docs, they should also be updated for consistency.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Modernize CLI, backend, and SDK code by removing redundant constructs, centralizing shared logic, and tightening type/URL handling while keeping behavior intact.
Bug Fixes:
Enhancements:
Documentation:
Tests: