-
Notifications
You must be signed in to change notification settings - Fork 40
Add missing default values to CLI argument descriptions in the import command #3059
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?
Conversation
Summary of ChangesHello @inv-jishnu, 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 enhances the clarity and completeness of the 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 improves the CLI help text by adding default values to the descriptions of four arguments, which is a good usability improvement. However, this manual approach has some drawbacks. It's applied inconsistently, as many other arguments with default values in ImportCommandOptions.java
are not updated. This also creates a maintenance issue, as the description can get out of sync with the defaultValue
if it's changed in the future.
A more robust solution is to use picocli
's built-in feature to display default values. By setting showDefaultValues = true
on the @CommandLine.Command
annotation in ImportCommand.java
, picocli
will automatically and consistently handle this for all options.
I recommend reverting the manual changes in this PR and adopting the automatic approach for better consistency and maintainability.
description = | ||
"Maximum number of records to be included in a single data chunk (default: 500)", |
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.
As detailed in the general feedback, manually adding the default value here is not ideal. It can lead to inconsistencies and maintenance issues. I recommend reverting this change in favor of enabling picocli
's automatic default value display feature, which will handle this consistently for all options.
description = "Maximum number of records to be included in a single data chunk",
Description
Added missing default values to the help descriptions of the following CLI arguments.
Previously, these arguments had default values defined in code, but they were not reflected in the help section, leading to incomplete information for users.
--data-chunk-size
--transaction-size
--split-log-mode
--data-chunk-queue-size
Related issues and/or PRs
NA
Changes made
Added missing default values to the description of the following CLI arguments in the import command options:
--data-chunk-size
--transaction-size
--split-log-mode
--data-chunk-queue-size
Checklist
Additional notes (optional)
NA
Release notes
Updated the import command help text to include default values for four CLI arguments.