feat: add --reinstall-tui flag to force reinstall rogue-tui #78
Conversation
This flag allows the user to force the reinstallation of the rogue-tui binary, even if it already exists. This is useful for development and testing purposes. Fixes qualifire-dev#74
Summary by CodeRabbit
WalkthroughAdds a --force-reinstall CLI flag and a Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as CLI (__main__)
participant I as RogueTuiInstaller
participant A as ExampleAgent (optional)
U->>C: rogue-ai [--force-reinstall]
C->>I: install_rogue_tui(force=args.force_reinstall)
note right of I `#DDEBF7`: New param: force\nforces redownload/reinstall when True
alt install succeeds
I-->>C: success
C->>A: continue/start (default mode)
C-->>U: proceed
else install fails
I-->>C: failure
C->>A: terminate & wait (if started)
C-->>U: exit with error
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rogue/common/tui_installer.py (1)
168-172: Update docstring to document the reinstall parameter.The method signature now includes a
reinstallparameter, but the docstring doesn't describe it. Users and maintainers should understand when and why to use this flag.Apply this diff to update the docstring:
def install_rogue_tui( self, upgrade: bool = False, reinstall: bool = False, ) -> bool: - """Install rogue-tui from GitHub releases if not already installed.""" + """Install rogue-tui from GitHub releases if not already installed. + + Args: + upgrade: Force upgrade even if already installed. + reinstall: Force reinstallation even if already installed. + + Returns: + bool: True if installation succeeded, False otherwise. + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rogue/__main__.py(2 hunks)rogue/common/tui_installer.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Format Python code with Black
Ensure code passes flake8 linting
Run mypy with the repository configuration for static typing
Run Bandit security checks using .bandit.yaml configuration
Use isort import conventions for import ordering
Add type hints to all function signatures
Follow PEP 8 naming (snake_case for variables/functions, PascalCase for classes)
Use try/except around code that may raise exceptions
Files:
rogue/common/tui_installer.pyrogue/__main__.py
🧬 Code graph analysis (1)
rogue/__main__.py (1)
rogue/common/tui_installer.py (2)
RogueTuiInstaller(17-206)install_rogue_tui(168-206)
🔇 Additional comments (4)
rogue/common/tui_installer.py (1)
176-176: LGTM!The early-exit logic correctly honors the
reinstallflag. Whenreinstall=True, the condition evaluates toFalse, bypassing the early return and proceeding with the download/installation flow.rogue/__main__.py (3)
56-61: LGTM!The CLI argument is properly defined with appropriate defaults and help text.
208-210: LGTM!The
reinstallflag is correctly passed to the installer in default mode, enabling forced reinstallation when requested.
257-257: Verify whether TUI mode should also support the reinstall flag.The TUI mode at line 257 invokes
install_rogue_tui()without passing thereinstallflag, while the default mode at lines 208-210 does pass it. This creates an inconsistency where users runningrogue tui --reinstall-tuiwon't get the expected behavior.Please clarify whether TUI mode should honor the
--reinstall-tuiflag. If so, apply this diff:elif args.mode == "tui": - if not RogueTuiInstaller().install_rogue_tui(): + if not RogueTuiInstaller().install_rogue_tui(reinstall=args.reinstall_tui): logger.error("Failed to install rogue-tui. Exiting.") sys.exit(1)If TUI mode should not support reinstall, consider documenting this limitation in the CLI help text or raising an error when
--reinstall-tuiis used withtuimode.
|
Docstring Coverage done too check it out |
yuval-qf
left a comment
yuval-qf
left a comment
There was a problem hiding this comment.
Thanks for this contribution! 💪
Please look at the comment that i've left
ktej721
force-pushed
the
feature/74-reinstall-tui
branch
from
6910854 to
643d6b6
Compare
|
i've updated the pr to combine the upgrade and reinstall flags into a single force flag in RogueTuiInstaller.install_rogue_tui, as you asked me to do.... i also renamed the --reinstall-tui command-line argument to --force-reinstall in rogue/main.py and connected it to the new force parameter. this simplifies the implementation while providing the same functionality. plz check it out. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rogue/__main__.py (1)
56-61: Consider enhancing the help text for better user experience.The help text could be more descriptive to explain when this flag is useful. For example: "Force reinstallation of rogue-tui even if already installed. Useful for development, testing, or recovering from a corrupted installation."
Apply this diff to enhance the help text:
parent_parser.add_argument( "--force-reinstall", action="store_true", default=False, - help="Force reinstallation of the TUI.", + help="Force reinstallation of rogue-tui even if already installed. Useful for development, testing, or recovering from a corrupted installation.", )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rogue/__main__.py(3 hunks)rogue/common/tui_installer.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rogue/common/tui_installer.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Format Python code with Black
Ensure code passes flake8 linting
Run mypy with the repository configuration for static typing
Run Bandit security checks using .bandit.yaml configuration
Use isort import conventions for import ordering
Add type hints to all function signatures
Follow PEP 8 naming (snake_case for variables/functions, PascalCase for classes)
Use try/except around code that may raise exceptions
Files:
rogue/__main__.py
🧬 Code graph analysis (1)
rogue/__main__.py (1)
rogue/common/tui_installer.py (2)
RogueTuiInstaller(17-209)install_rogue_tui(168-209)
🔇 Additional comments (2)
rogue/__main__.py (2)
208-213: LGTM!The
forceparameter is correctly passed to the installer, and the error handling appropriately terminates the example process before exiting if installation fails.
255-259: LGTM!The
forceparameter is correctly passed to the installer, and the error handling is appropriate. The example process cleanup will be handled by the finally block (lines 265-268) when the installation fails and exits.
|
I guess there are some errors, let me address them accordingly and update. |
Thanks! |
|
Sure thing and I am working on it too |
ktej721
had a problem deploying
to
rogue-sanity-ci-secrets
GitHub Actions
Failure
|
Hi @ktej721, is there any update regarding this PR? |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rogue/__main__.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Format Python code with Black
Ensure code passes flake8 linting
Run mypy with the repository configuration for static typing
Run Bandit security checks using .bandit.yaml configuration
Use isort import conventions for import ordering
Add type hints to all function signatures
Follow PEP 8 naming (snake_case for variables/functions, PascalCase for classes)
Use try/except around code that may raise exceptions
Files:
rogue/__main__.py
🧬 Code graph analysis (1)
rogue/__main__.py (1)
rogue/common/tui_installer.py (2)
RogueTuiInstaller(17-209)install_rogue_tui(168-209)
🔇 Additional comments (2)
rogue/__main__.py (2)
70-75: CLI flag looks good.The implementation follows the existing argument patterns and correctly defaults to
False. The help text is clear and concise.
282-284: TUI mode integration looks correct.The
forceparameter is properly passed, and the error handling aligns with the expected behavior for TUI mode. The example process cleanup in thefinallyblock ensures proper resource management.
|
|
||
| # Step 1: Install rogue-tui if needed | ||
| if not RogueTuiInstaller().install_rogue_tui(): | ||
| if not RogueTuiInstaller().install_rogue_tui(force=args.force_reinstall): |
There was a problem hiding this comment.
🧩 Analysis chain
Verify all call sites have been updated with the force parameter.
The usage here is correct. However, based on the PR comments, there appears to be at least one more call to install_rogue_tui() in rogue/common/update_checker.py that may need updating to align with the new signature.
Run the following script to find all call sites:
🏁 Script executed:
#!/bin/bash
# Description: Find all calls to install_rogue_tui in the codebase to ensure the new parameter is handled consistently.
# Search for all invocations of install_rogue_tui method
rg -n -C3 '\.install_rogue_tui\('Length of output: 1353
Let me search for the install_rogue_tui function definition to verify the expected parameters:The web search results are not relevant to the specific codebase. Let me search the codebase for the function definition directly:
</shell>
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the definition of install_rogue_tui to verify the expected parameters
ast-grep --pattern 'def install_rogue_tui($$$) {
$$$
}'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Alternative: search for the method definition with rg
rg -n -A10 'def install_rogue_tui'Length of output: 766
🏁 Script executed:
#!/bin/bash
# Search in Python files for the function signature
rg -n -B2 -A8 'def install_rogue_tui'Length of output: 714
🏁 Script executed:
#!/bin/bash
# Check the update_checker.py file to see context around line 220
cat rogue/common/update_checker.py | sed -n '210,230p'Length of output: 815
🏁 Script executed:
#!/bin/bash
# Check the RogueTuiInstaller class definition
rg -n -B3 -A15 'class RogueTuiInstaller'Length of output: 1213
🏁 Script executed:
#!/bin/bash
# Verify if there are any changes to the function signature in the PR
rg -n 'def install_rogue_tui' --type pyLength of output: 95
Fix call to install_rogue_tui() in rogue/common/update_checker.py—parameter name mismatch.
The function signature expects a force parameter, but line 220 in rogue/common/update_checker.py passes upgrade=True instead. Update this call to use force=True:
# rogue/common/update_checker.py:220-222
RogueTuiInstaller().install_rogue_tui(
force=True,
)🤖 Prompt for AI Agents
In rogue/common/update_checker.py around lines 220-222, the call to
RogueTuiInstaller().install_rogue_tui(...) uses the wrong keyword argument name
(upgrade=True); change that argument to force=True so it matches the function
signature (i.e., call RogueTuiInstaller().install_rogue_tui(force=True)), and
run tests/lint to ensure no other call sites use the incorrect keyword.
3 participants
This allows the user to force the reinstallation of the rogue-tui binary, even if it already exists. This is useful for development and testing purposes.
Fixes #74