remove legacy compile #841
Conversation
WalkthroughThe pull request removes two legacy compilation scripts and refactors the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI (compile)
participant Compiler
User->>CLI (compile): Invoke compile --chronon-root
CLI (compile)->>CLI (compile): Add chronon_root to sys.path
CLI (compile)->>CLI (compile): Check for teams.py/teams.json
CLI (compile)->>Compiler: compiler.compile()
Compiler-->>CLI (compile): Compilation results
CLI (compile)-->>User: Output results
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (4)
api/python/ai/chronon/repo/compile.py(1 hunks)api/python/ai/chronon/repo/compilev2.py(0 hunks)api/python/ai/chronon/repo/compilev3.py(0 hunks)api/python/ai/chronon/repo/zipline.py(2 hunks)
💤 Files with no reviewable changes (2)
- api/python/ai/chronon/repo/compilev3.py
- api/python/ai/chronon/repo/compilev2.py
🧰 Additional context used
🪛 Pylint (3.3.7)
api/python/ai/chronon/repo/compile.py
[error] 56-56: No value for argument 'chronon_root' in function call
(E1120)
🔇 Additional comments (3)
api/python/ai/chronon/repo/zipline.py (1)
7-7: LGTM - Clean migration from legacy compile.Import and command registration updated correctly.
Also applies to: 47-47
api/python/ai/chronon/repo/compile.py (2)
18-32: LGTM - Proper sys.path handling.Clean logic for adding chronon_root to Python path with appropriate console feedback.
35-52: LGTM - Good validation and delegation pattern.Validates teams file existence and properly delegates to Compiler abstraction.
|
|
||
| if __name__ == "__main__": | ||
| extract_and_convert() | ||
| compile() |
There was a problem hiding this comment.
Fix missing function argument.
The compile() function requires chronon_root parameter but none is provided.
- compile()
+ compile(chronon_root=os.getcwd())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| compile() | |
| - compile() | |
| + compile(chronon_root=os.getcwd()) |
🧰 Tools
🪛 Pylint (3.3.7)
[error] 56-56: No value for argument 'chronon_root' in function call
(E1120)
🤖 Prompt for AI Agents
In api/python/ai/chronon/repo/compile.py at line 56, the call to the compile()
function is missing the required chronon_root argument. Update the function call
to include the chronon_root parameter with the appropriate value to match the
function's signature.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
api/python/ai/chronon/repo/zipline.py (1)
7-8: Avoid shadowing Python’s built-incompile.Importing the CLI command as
compile(and registering it with the same name) hides the built-incompile()which might be needed elsewhere and confuses IDE auto-completion.-from ai.chronon.repo.compile import compile +from ai.chronon.repo.compile import compile as compile_cmd … -zipline.add_command(compile) +zipline.add_command(compile_cmd)Also applies to: 47-48
api/python/ai/chronon/repo/compile.py (3)
18-23: Name collision with built-incompile().The decorator replaces
compilewith a Click wrapper, yet calling it directly at L56 still masks Python’s built-in. Rename for clarity.-@click.command(name="compile") -def compile(chronon_root): +@click.command(name="compile") +def cli_compile(chronon_root): … -if __name__ == "__main__": - compile() +if __name__ == "__main__": + cli_compile()Also applies to: 56-56
22-30: Path check brittle; use absolute form.
chronon_root not in sys.pathfails if the same path appears with a different casing or trailing slash.- if chronon_root not in sys.path: + abspath = os.path.abspath(chronon_root) + if abspath not in sys.path: … - sys.path.append(chronon_root) + sys.path.append(abspath)
20-20: Strayprint().Blank print adds noise; rely on
consolefor output.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/python/ai/chronon/repo/compile.py(1 hunks)api/python/ai/chronon/repo/compilev2.py(0 hunks)api/python/ai/chronon/repo/compilev3.py(0 hunks)api/python/ai/chronon/repo/zipline.py(2 hunks)
💤 Files with no reviewable changes (2)
- api/python/ai/chronon/repo/compilev3.py
- api/python/ai/chronon/repo/compilev2.py
🧰 Additional context used
🪛 Pylint (3.3.7)
api/python/ai/chronon/repo/compile.py
[error] 56-56: No value for argument 'chronon_root' in function call
(E1120)
| def __compile(chronon_root): | ||
| if chronon_root: | ||
| chronon_root_path = os.path.expanduser(chronon_root) | ||
| os.chdir(chronon_root_path) |
There was a problem hiding this comment.
Guard against missing directory before chdir.
Calling os.chdir on a non-existent path raises FileNotFoundError. Fail early with a clear Click error.
- os.chdir(chronon_root_path)
+ if not os.path.isdir(chronon_root_path):
+ raise click.ClickException(f"Directory not found: {chronon_root_path}")
+ os.chdir(chronon_root_path)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __compile(chronon_root): | |
| if chronon_root: | |
| chronon_root_path = os.path.expanduser(chronon_root) | |
| os.chdir(chronon_root_path) | |
| def __compile(chronon_root): | |
| if chronon_root: | |
| chronon_root_path = os.path.expanduser(chronon_root) | |
| if not os.path.isdir(chronon_root_path): | |
| raise click.ClickException(f"Directory not found: {chronon_root_path}") | |
| os.chdir(chronon_root_path) |
🤖 Prompt for AI Agents
In api/python/ai/chronon/repo/compile.py around lines 35 to 38, before calling
os.chdir, add a check to verify that the directory exists using os.path.isdir.
If the directory does not exist, raise a clear Click exception to fail early
with an informative error message instead of allowing os.chdir to raise
FileNotFoundError.
tchow-zlai
force-pushed
the
tchow/delete-cli-unused-files
branch
from
9a95159 to
882a50a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/python/test/test_compile.py (1)
12-12: Using private function in tests.Testing
__compile(private function) may break if internal implementation changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/python/test/test_compile.py(1 hunks)api/python/test/test_compilev3.py(0 hunks)
💤 Files with no reviewable changes (1)
- api/python/test/test_compilev3.py
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: cloud_aws_tests
- GitHub Check: service_tests
- GitHub Check: cloud_gcp_tests
- GitHub Check: api_tests
- GitHub Check: service_commons_tests
- GitHub Check: online_tests
- GitHub Check: flink_tests
- GitHub Check: aggregator_tests
- GitHub Check: python_tests
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (2)
api/python/test/test_compile.py (2)
36-39: Complex patch setup looks correct.The multiple patch decorators properly mock the dependencies for isolated testing.
55-57: Good relative path validation.The assertions correctly verify both the expected path format and that it's not absolute.
| def test_compile(repo): | ||
| os.chdir(repo) | ||
| results = __compile(chronon_root=repo) | ||
| assert len(results) != 0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider test isolation concerns.
Changing working directory can affect other tests if they run in parallel or don't reset state.
def test_compile(repo):
- os.chdir(repo)
- results = __compile(chronon_root=repo)
+ original_cwd = os.getcwd()
+ try:
+ os.chdir(repo)
+ results = __compile(chronon_root=repo)
+ finally:
+ os.chdir(original_cwd)
assert len(results) != 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_compile(repo): | |
| os.chdir(repo) | |
| results = __compile(chronon_root=repo) | |
| assert len(results) != 0 | |
| def test_compile(repo): | |
| original_cwd = os.getcwd() | |
| try: | |
| os.chdir(repo) | |
| results = __compile(chronon_root=repo) | |
| finally: | |
| os.chdir(original_cwd) | |
| assert len(results) != 0 |
🤖 Prompt for AI Agents
In api/python/test/test_compile.py around lines 10 to 13, the test changes the
working directory without restoring it, which can cause side effects on other
tests. Modify the test to save the current working directory before changing it
and restore it after the test completes, ideally using a try-finally block or a
context manager to ensure the original directory is reset regardless of test
outcome.
nikhil-zlai
left a comment
nikhil-zlai
left a comment
There was a problem hiding this comment.
amazing! thanks for doing this!
Summary
Checklist
Summary by CodeRabbit
Refactor
compilecommand with minimal options.compilecommand instead of previous versions.Chores