-
Notifications
You must be signed in to change notification settings - Fork 29
[cli] better definition of background CLI commands + add more background commands #728
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: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @EskoDijk, 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 refines how background CLI commands are identified and managed within the system. It introduces a more structured approach to defining these commands and expands the set of commands that are recognized as background operations, ensuring better command handling and system responsiveness. Highlights
Ignored Files
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 refactors the definition of background CLI commands by moving them to a string slice, which greatly improves maintainability and readability. It also correctly expands the list of recognized background commands. This is a solid improvement. I have one suggestion to make the regular expression generation even more robust.
a5f4d83 to
96cb395
Compare
|
@jwhui Just fixed and rebased this PR - should now be ready to review. This adds more OT CLI commands (that run in the 'background' producing results onto the CLI) to the set of working background commands. |
aefe270 to
0b895ab
Compare
…round commands Improves the detection of background OT CLI commands by checking command output of potential OT CLI background commands for the 'Done' string at the end. If 'Done' is present, the command is marked as a foreground (non-background) command at that point. This allows a simpler definition of the list of (potential) background commands and fixes the issue that some commands may not work correctly when used with the 'async' flag. Also a unit test is added to exercise various background commands.
0b895ab to
35aaaef
Compare
|
@jwhui Issue in the commits was fixed 4 days ago (there was a test commit unintentionally included). Now ready for approval or another review if needed. |
|
/gemini review |
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 refactors the definition of background CLI commands, making them clearer and easier to maintain by moving them into a string slice. It also expands the list of background commands. The logic for handling these commands is improved to correctly identify when a potential background command behaves like a foreground command by returning 'Done' immediately. New tests have been added to cover this new functionality.
I've found a couple of minor issues: one in the implementation of background command detection logic which could fail with certain output formats, and another in the test suite with an incorrect assertion. My review includes suggestions to fix these.
bug fix in final 'Done' string detection - now ignores any newlines or white space that may follow after the final 'Done' line.
bugfix in CLI regexp's; updated tests.
|
@jwhui Now updated based on the last Gemini review comments. |
|
@jwhui We have 2 test failures on the compiled (webpack) Javascript result, but this PR doesn't change anything there! In any case it shouldn't affect this PR I think. |
Now created issue #737 to record the details for this issue. Not related to the present PR. |
Some OT CLI commands were missing as 'background' commands - this adds a few.
Also the list of background commands is more clear now with one command per line.