[#8300] fix(cli): add missing space in RemoveModelVersionProperty output #9557
Conversation
|
Hi, this PR fixes the missing space in the CLI output as discussed in #8300. |
There was a problem hiding this comment.
Pull request overview
This PR aims to fix a formatting issue in the CLI output by adding missing spaces before property names in the RemoveModelVersionProperty command. However, the changes reveal a deeper issue with message redundancy, and include an undocumented change to an unrelated file.
Key changes:
- Added missing spaces in output messages for RemoveModelVersionProperty (lines 102 and 105)
- Changed ellipsis character representation in Column.java from literal to Unicode escape
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| clients/cli/src/main/java/org/apache/gravitino/cli/commands/RemoveModelVersionProperty.java | Adds missing spaces before "property" keyword in output messages, fixing formatting but revealing redundant "property" text |
| clients/cli/src/main/java/org/apache/gravitino/cli/outputs/Column.java | Changes ellipsis character from literal '…' to Unicode escape '\u2026' (undocumented change) |
|
|
||
| if (alias != null) { | ||
| printInformation(model + " alias " + alias + "property " + property + " property removed."); | ||
| printInformation(model + " alias " + alias + " property " + property + " property removed."); |
There was a problem hiding this comment.
The output message contains redundant text with "property" appearing twice. The message reads "model alias aliasA property key property removed." which should likely be "model alias aliasA property key removed." Consider removing one instance of "property" to improve message clarity.
| } else { | ||
| printInformation( | ||
| model + " version " + version + "property " + property + " property removed."); | ||
| model + " version " + version + " property " + property + " property removed."); |
There was a problem hiding this comment.
The output message contains redundant text with "property" appearing twice. The message reads "model version 1 property key property removed." which should likely be "model version 1 property key removed." Consider removing one instance of "property" to improve message clarity.
| public class Column { | ||
| /** The character used to indicate that a cell has been truncated. */ | ||
| public static final char ELLIPSIS = '…'; | ||
| public static final char ELLIPSIS = '\u2026'; |
There was a problem hiding this comment.
This change from character literal '…' to Unicode escape '\u2026' was not mentioned in the PR description. While both represent the same ellipsis character and the Unicode escape is more explicit, this appears to be an unrelated change that should either be documented in the PR description or submitted as a separate change. If this is intentional, please update the PR description to explain why this change was made.
| public static final char ELLIPSIS = '\u2026'; | |
| public static final char ELLIPSIS = '…'; |
1 participant
What changes were proposed in this pull request?
This pull request fixes a formatting issue in the CLI output where a space
was missing before the model version property name in
RemoveModelVersionProperty.java.
Why are the changes needed?
The CLI output was incorrectly formatted due to a missing space before the
property name, which reduced readability and resulted in inconsistent
output formatting. This change improves the clarity of the CLI output.
Fixes #8300
Does this PR introduce any user-facing change?
Yes. The CLI output formatting is corrected by adding a missing space before
the model version property name. No APIs or configuration properties are
changed.
How was this patch tested?
The change was manually verified by running the CLI command and confirming
that the output now includes the correct spacing. No automated tests were
added as this is a minor formatting fix.