Set --log-raw-record default to true for all failure logs #3185
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 standardizes the behavior of failure logging by setting 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
|
![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 updates the default value for the --log-raw-record option to true in the import command, making the behavior consistent with what was expected. It also standardizes the deprecation comments for several options, improving code documentation. The changes are logical, but I've identified a point of inconsistency with the core options class and a lack of test coverage for the new default value, which should be addressed.
| description = "Include the original source record in the log file output (default: true)", | ||
| defaultValue = "true") |
There was a problem hiding this comment.
I have a couple of points regarding this change:
-
Inconsistency with Core Options: While changing the default value here to
truealigns with the PR's goal, it creates an inconsistency with the coreImportOptionsclass, wherelogRawRecordstill defaults tofalse. This means the default behavior will differ depending on whether the tool is used via the CLI or programmatically. For consistency, please consider also updating the default indata-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/ImportOptions.java. -
Missing Test Coverage: Changing a default value is a behavioral change that should be covered by tests.
ImportCommandTest.javaappears to be missing tests for the--log-raw-recordoption. Please add a test case to verify thatlogRawRecorddefaults totruewhen the option is not specified in the command arguments.
There was a problem hiding this comment.
I have updated changes based on suggestion in 58c5051
ypeckstadt
changed the title
--log-raw-record to true1 participant
Description
This PR updates the default value of
--log-raw-recordto true by default so that raw records are logged in failure log unless the user specifies otherwise.It was set to false by default but that is not really the expected behavior users want. The feature will not be deprecated though so the users have the option to still disable it.
This PR also fixes a few comments that mention the wrong target version for deprecation.
Related issues and/or PRs
NA
Changes made
--log-raw-recordto trueChecklist
Additional notes (optional)
NA
Release notes
Update default value for
--log-raw-recordto true