refactor(gcli): clean up and overhaul most parts of gcli#5057
refactor(gcli): clean up and overhaul most parts of gcli#5057liferooter merged 38 commits intomasterfrom
gcli#5057Conversation
Summary of ChangesHello @liferooter, 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 undertakes a comprehensive refactor 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.
Code Review
This pull request is a significant and well-executed refactoring of the gcli crate. The changes greatly improve the codebase by restructuring it from a library to a binary, cleaning up the CLI, unifying output, and improving type safety with FromStr/Display implementations for hex-like types. The test suite has also been modernized, which is a welcome improvement.
My review has identified a few points for further improvement. I've found a critical portability issue in the update command that would prevent it from compiling on non-Unix platforms. Additionally, there are a few opportunities to enhance code clarity and maintainability, a minor performance improvement by removing a redundant clone, and some inconsistencies in output logging. I've also flagged a test that appears to be outdated and will likely fail.
Overall, this is a high-quality contribution that moves gcli in a very positive direction. Addressing the points in my review will help ensure the tool is robust, portable, and even more maintainable.
ark0f
left a comment
There was a problem hiding this comment.
Consider to use trycmd crate for CLI testing. Some commands have quite complex logic
This reverts commit 17cad50.
gcliexcept the one connected withgringFromStr/Displayimplementations for hex-like types instead of manual usage ofhexgcliCLIgclioutputgsdkchanges ingcligclicrate fromlib + named binarytolib + binarygclitests more straightforward, remove advanced wrapper aroundgclifrom themgclitests as redundant.gcliis a CLI wrapper aroundgsdk, no need to testgsdkagain