-
Notifications
You must be signed in to change notification settings - Fork 3
Refactor client CLIs #324
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
base: master
Are you sure you want to change the base?
Refactor client CLIs #324
Conversation
Summary of ChangesHello @jnmt, 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 significantly refactors the command-line interface (CLI) tools for both the general client and the table-store client. By introducing abstract base classes, 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a great refactoring that introduces AbstractClientCommand and AbstractTableStoreCommand to centralize common CLI logic, such as service creation, exception handling, and resource cleanup. This significantly reduces boilerplate code in individual command classes and improves maintainability.
My review has identified a few areas for improvement to enhance consistency and robustness:
- Final methods: The
call()method in the new abstract classes should be marked asfinalto prevent subclasses from accidentally bypassing the common execution logic. - Consistent Exception Handling: In a few places, exceptions are caught and handled in a way that bypasses the new centralized, JSON-based error reporting. Specifically:
IOExceptioninContractsRegistrationandFunctionsRegistrationshould be wrapped in aClientExceptionand re-thrown.- Argument parsing exceptions in
LedgerValidationshould also be wrapped and re-thrown to ensure all errors are reported to the user in a consistent JSON format.
Overall, this is a solid improvement. Addressing these points will make the refactoring even more robust and consistent.
client/src/main/java/com/scalar/dl/client/tool/AbstractClientCommand.java
Show resolved
Hide resolved
| } catch (IOException e) { | ||
| System.err.printf(ClientError.WRITING_RESULT_TO_FILE_FAILED.buildMessage(e.getMessage())); | ||
| printStackTrace(e); | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IOException is caught and handled here by printing a raw string to System.err and returning 1. This bypasses the centralized JSON error reporting mechanism in AbstractClientCommand. To maintain consistency in error reporting, this exception should be wrapped in a ClientException and re-thrown, allowing the abstract class to handle it.
} catch (IOException e) {
throw new ClientException(ClientError.WRITING_RESULT_TO_FILE_FAILED, e, e.getMessage());
}
client/src/main/java/com/scalar/dl/client/tool/FunctionsRegistration.java
Show resolved
Hide resolved
table-store/src/main/java/com/scalar/dl/tablestore/client/tool/AbstractTableStoreCommand.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors command-line tools in both the client and table-store modules by extracting common command execution logic into abstract base classes (AbstractClientCommand and AbstractTableStoreCommand). The refactoring reduces code duplication by centralizing factory creation, service initialization, error handling, and resource cleanup.
- Introduced abstract base classes to handle common command lifecycle (service creation, error handling, factory cleanup)
- Simplified test methods to use the new
execute()method directly or callcall(factory)with proper mocking - Fixed error output to use
System.errinstead ofSystem.outin clientLedgerValidation - Added new error type for file writing failures
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| AbstractTableStoreCommand.java | New abstract base class consolidating common logic for table-store commands |
| AbstractClientCommand.java | New abstract base class consolidating common logic for client commands |
| StatementExecution.java, LedgerValidation.java, Bootstrap.java (table-store) | Refactored to extend AbstractTableStoreCommand and implement execute() method |
| Multiple client tool classes | Refactored to extend AbstractClientCommand and implement execute() method |
| Test files | Updated tests to use execute() for happy paths and call(factory) for error scenarios |
| ClientError.java | Added WRITING_RESULT_TO_FILE_FAILED error enum |
| LedgerValidation.java (client) | Changed error output from System.out to System.err |
| FunctionsRegistration.java, ContractsRegistration.java | Updated file writing to use Files.newOutputStream and added IOException handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
client/src/main/java/com/scalar/dl/client/tool/FunctionsRegistration.java
Outdated
Show resolved
Hide resolved
client/src/main/java/com/scalar/dl/client/tool/ContractsRegistration.java
Outdated
Show resolved
Hide resolved
josh-wong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!🙇🏻♂️
Description
This PR refactors the client CLI (and TableStore as well) commands before adding new commands for creating namespaces and so on. Basically, it follows what I did in the HashStore CLI, adding an abstract command class.
Related issues and/or PRs
N/A
Changes made
Checklist
Additional notes (optional)
N/A
Release notes
Refactored client CLI commands.