feature/refactor-versions#88
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbitRelease Notes
WalkthroughAdds a single-source-of-truth VERSION file and wires it into Go builds and runtime and Python packaging/runtime; introduces a package-exported Changes
Sequence Diagram(s)sequenceDiagram
participant User as Developer / CLI user
participant CLI as rogue-tui (root cmd)
participant TUIpkg as tui package (tui.Version)
User->>CLI: run 'rogue-tui --version' or 'rogue-tui version'
CLI->>TUIpkg: read tui.Version
TUIpkg-->>CLI: return Version (ldflags value or default)
CLI-->>User: print Version and exit
sequenceDiagram
participant Make as Makefile / Build
participant VERSION as VERSION file
participant Binary as Built binary
Make->>VERSION: read ../../VERSION (or fallback to dev)
Make->>Binary: set ldflags github.com/rogue/tui/internal/tui.Version=vX.Y.Z
Binary->>Binary: runtime uses tui.Version for CLI and app
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
🔇 Additional comments (1)
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: 3
🧹 Nitpick comments (4)
packages/tui/Makefile (1)
14-14: Remove hardcoded "v" prefix to avoid cross-tool inconsistency.Current ldflags result in v0.1.12, while Python exposes 0.1.12. Prefer injecting the raw version and let UIs decide how to display it.
Apply this diff:
-LDFLAGS=-ldflags "-X github.com/rogue/tui/internal/tui.Version=v$(VERSION) -X main.commit=$(COMMIT) -X main.date=$(DATE)" +LDFLAGS=-ldflags "-X github.com/rogue/tui/internal/tui.Version=$(VERSION) -X main.commit=$(COMMIT) -X main.date=$(DATE)"Also ensure main.commit and main.date are still defined in package main, otherwise the build will fail when using -X.
pyproject.toml (1)
48-51: Hatch version sourcing looks correct. Consider including VERSION in artifacts.Build-time extraction from VERSION is fine. For transparency/debugging, consider including VERSION in the sdist/wheel so tools/users can inspect it after install.
Would you like me to propose hatchling config to force-include VERSION in both sdist and wheel?
packages/tui/internal/tui/version.go (1)
3-5: Align default with raw semver (drop "v" prefix).Defaulting to "vdev" conflicts with Python’s "0.0.0-dev" style and raw VERSION content. Prefer "dev" here to keep a single convention.
-// Default to "dev" if not set during build -var Version = "vdev" +// Default to "dev" if not set during build +var Version = "dev"packages/tui/cmd/rogue/main.go (1)
30-55: Version handling works; consider simplifying via Cobra’s built-in Version.Optional: set rootCmd.Version = tui.Version and drop the custom PreRun and version subcommand, or keep the subcommand and remove PreRun to avoid duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
VERSION(1 hunks)packages/tui/Makefile(1 hunks)packages/tui/cmd/rogue/main.go(1 hunks)packages/tui/internal/tui/app.go(1 hunks)packages/tui/internal/tui/version.go(1 hunks)pyproject.toml(3 hunks)rogue/__init__.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
pyproject.toml
📄 CodeRabbit inference engine (AGENTS.md)
Manage dependencies with uv and declare them in pyproject.toml
Files:
pyproject.toml
**/*.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/__init__.py
🧬 Code graph analysis (2)
packages/tui/internal/tui/app.go (1)
packages/tui/internal/tui/version.go (1)
Version(5-5)
packages/tui/cmd/rogue/main.go (2)
packages/tui/internal/commands/commands.go (2)
RunTUI(11-16)NewTUICommand(19-27)packages/tui/internal/tui/version.go (1)
Version(5-5)
🔇 Additional comments (1)
packages/tui/internal/tui/app.go (1)
278-279: Good switch to centralized Version.Initializing from the package-level Version variable aligns the UI with build-time injected version.
Please confirm the intended "v" prefix convention matches the CLI and Python outputs.
yuval-qf
left a comment
yuval-qf
left a comment
There was a problem hiding this comment.
Thanks for this great contribution! This will without a doubt help us manage the versions more efficiently!
A few notes before approving:
- In go, indentation is usually done in tabs instead of spaces. I've noticed that you used spaces. Please convert back to tabs.
From the go.dev docs:
Indentation
We use tabs for indentation and gofmt emits them by default. Use spaces only if you must.
- Please look at the two notes from coderabbit. His notes are valid
- If you can, please use the same versioning logic in the
rogue_sdk, located insdks/python/pyproject.toml - Please fix the conflicts :)
|
heyy @yuval-qf, I noticed does this sound correct to you? just wanted to double-check before I push the changes. |
Hey @puwun , Thanks! |
|
the python sdk now uses the same versioning logic and pulls from the root |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
packages/tui/cmd/rogue/main.go
Outdated
| fmt.Fprintf(os.Stderr, "Error: %v\n", err) | ||
| os.Exit(1) | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Bug: Go Code Formatting Violation
The Go code in main.go now uses spaces for indentation instead of tabs throughout the file. This goes against Go's standard formatting conventions and was also noted in PR review feedback.
|
@yuval-qf , i have centralized versioning so both rogue-ai and rogue-ai-sdk use the single root So, the key challenge was that the rogue-ai-sdk is a standalone package and can't access the project's file structure after a user installs it. To solve this, I now have embedded the version number into the sdk's package metadata during the build process itself. To improve the local development workflow, I've also updated the root [tool.uv.sources]
rogue-ai-sdk = { path = "sdks/python", editable = true }This tells our tools to use the local sdk code instead of downloading it from a repository, this gives us a true monorepo workflow where changes to the sdk are instantly available to the main application, which speeds up development and testing 🚀🚀 I also took care of the Go code formatting issue in Pls take a look and suggest if any changes needed |
Thanks @puwun, this solution seems great! It seems that the |
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 (2)
rogue/common/version.py(1 hunks)sdks/python/rogue_sdk/__init__.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rogue/common/version.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:
sdks/python/rogue_sdk/__init__.py
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
sdks/python/rogue_sdk/__init__.py (1)
60-67: Good: typed version and specific exception.This implements the earlier ask: add type annotation and catch PackageNotFoundError instead of broad Exception. Nice.
🧹 Nitpick comments (1)
sdks/python/rogue_sdk/__init__.py (1)
54-57: Remove unused TYPE_CHECKING stub (and import) or put it to use.The empty TYPE_CHECKING block adds noise and will trigger F401 once removed from use. Either drop it or use it for type-only imports. Also, isort will want stdlib imports (typing) before local package imports. As per coding guidelines.
Apply this minimal cleanup:
-from typing import TYPE_CHECKING - -if TYPE_CHECKING: - pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sdks/python/rogue_sdk/__init__.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:
sdks/python/rogue_sdk/__init__.py
🔇 Additional comments (1)
sdks/python/rogue_sdk/__init__.py (1)
61-67: The review comment is incorrect and should be disregarded.The package requires Python >= 3.9, and
importlib.metadatahas been available in the standard library since Python 3.8. The original code is already correct and appropriate for the stated requirements. There is no need to add a fallback to theimportlib_metadatabackport, as it would only add unnecessary complexity.The dist name "rogue-ai-sdk" is also verified as correct.
Likely an incorrect or invalid review comment.
3 participants
Description
This PR introduces a centralized
VERSIONfile to serve as the single source of truth for version management across the Rogue project. All components now read the version from this file, removing the need to manually update version numbers in multiple places.Motivation and Context
Previously, version numbers were hardcoded in three different files:
packages/tui/internal/tui/app.gorogue/__init__.pypyproject.tomlThis led to unnecessary maintenance overhead and inconsistencies.
Type of Change
Changes Made
VERSIONfile as single source of truth containing version0.1.12rogue/__init__.pyto read version from VERSION file at runtime with fallback to package metadatapackages/tui/internal/tui/version.goto use build-time variable injection instead of hardcoded valuepackages/tui/Makefileto read VERSION file and inject version via ldflagspackages/tui/cmd/rogue/main.goto addversioncommand and--versionflagpyproject.tomlto use hatch version plugin with VERSION fileScreenshots/Examples
Checklist
uv run black .to format my codeuv run flake8 .and fixed all issuesuv run mypy --config-file .mypy.ini .and addressed type checking issuesuv run bandit -c .bandit.yaml -r .for security checksuv run pytestand all tests passRelated Issues/PRs
Test Steps: