Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🧹 Nitpick comments (1)
pysyncrosim/scenario.py (1)
623-654: Consider restructuring to avoidreturninsidefinally.The static analysis tool flags the
return result_scnon line 650 inside thefinallyblock (Ruff B012). While your current logic is correct (the return only executes whenerror_msg is None), havingreturninsidefinallyis unconventional and can be confusing for future maintainers.Consider moving the success-path logic outside the
finallyblock by using a result variable that gets returned after thetry/except/finally.♻️ Suggested refactor to move return outside finally
def run(self, copy_external_inputs=False): # ... docstring ... args = ["--run", "--lib=%s" % self.library.location, "--sid=%d" % self.__sid] if copy_external_inputs is True: args += ["--copyextfiles=yes"] error_msg = None + result_scn = None try: print(f"Running Scenario [{self.sid}] {self.name}") self.library.session._Session__call_console(args) except RuntimeError as e: if "You must be signed in" in str(e): error_msg = ("you must be signed in to SyncroSim. " "Use session.sign_in() to sign in.") elif "There has been an issue with your SyncroSim license file" in str(e): error_msg = "there has been an issue with your SyncroSim license file." else: error_msg = str(e) finally: - # Reset Project Scenarios self.project._Project__scenarios = None # Reset results self.__results = None - + + # Retrieve Results Scenario ID + # Also resets scenarios and results info + results_df = self.results() + + if (not results_df.empty): + result_id = results_df["ScenarioId"].values[-1] + + if error_msg is not None: + raise RuntimeError(f"Run failed for Scenario [{result_id}] " + f"{self.name}: {error_msg}") + else: + print("Run successful") + # Retrieve Results Scenario ID - # Also resets scenarios and results info - results_df = self.results() - - if (not results_df.empty): - - result_id = results_df["ScenarioId"].values[-1] - - if error_msg is not None: - raise RuntimeError(f"Run failed for Scenario [{result_id}] " - f"{self.name}: {error_msg}") - else: - print("Run successful") - - # Return Results Scenario - result_scn = self.library.scenarios(project=self.project, - name=None, - sid=result_id) - - return result_scn - - elif error_msg is not None: - raise RuntimeError(f"Run failed for Scenario [{self.sid}] " - f"{self.name}: {error_msg}") + result_scn = self.library.scenarios(project=self.project, + name=None, + sid=result_id) + + elif error_msg is not None: + raise RuntimeError(f"Run failed for Scenario [{self.sid}] " + f"{self.name}: {error_msg}") + + return result_scn🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pysyncrosim/scenario.py` around lines 623 - 654, The finally block currently returns result_scn (Ruff B012); instead, declare a local variable (e.g., final_result = None) before the try, assign final_result = result_scn inside the finally when successful (preserve resetting self.project._Project__scenarios and self.__results and the results() call and prints/raises using error_msg), and move the actual "return final_result" to after the try/except/finally so no return occurs inside finally; reference the results() call, result_scn assignment via self.library.scenarios(..., sid=result_id), and the error_msg/sid/name-based RuntimeError branches when reorganizing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pysyncrosim/scenario.py`:
- Around line 652-654: The function currently falls through (implicit None) when
results_df.empty is True and error_msg is None; update the end-of-run handling
so it returns an explicit Scenario object and warns instead of returning None:
when results_df.empty and error_msg is None, emit a warning (e.g., using
warnings.warn with context including self.sid and self.name) and then return
self (the Scenario) so the return type matches the docstring; adjust the block
surrounding results_df.empty and error_msg checks (referencing results_df.empty,
error_msg, self.sid, self.name, and the Scenario return) to implement this
explicit warning+return.
In `@tests/test_pysyncrosim.py`:
- Around line 15-17: The test uses a hardcoded Windows-only path in
git_repo_path which breaks portability; replace usage in git_repo_path,
lib_path, and lib_backup_path so the repository root is discovered dynamically
(for example read from an environment variable like REPO_ROOT via
os.environ.get('REPO_ROOT') with a sensible fallback) or compute it relative to
the test file using os.path.dirname(__file__) and os.path.join to construct
lib_path and lib_backup_path; update the references in the test to use this
dynamic repo root so the tests run on different machines and CI.
---
Nitpick comments:
In `@pysyncrosim/scenario.py`:
- Around line 623-654: The finally block currently returns result_scn (Ruff
B012); instead, declare a local variable (e.g., final_result = None) before the
try, assign final_result = result_scn inside the finally when successful
(preserve resetting self.project._Project__scenarios and self.__results and the
results() call and prints/raises using error_msg), and move the actual "return
final_result" to after the try/except/finally so no return occurs inside
finally; reference the results() call, result_scn assignment via
self.library.scenarios(..., sid=result_id), and the error_msg/sid/name-based
RuntimeError branches when reorganizing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a57591d-acf2-479a-b4b1-8b5bcef683aa
📒 Files selected for processing (2)
pysyncrosim/scenario.pytests/test_pysyncrosim.py
| elif error_msg is not None: | ||
| raise RuntimeError(f"Run failed for Scenario [{self.sid}] " | ||
| f"{self.name}: {error_msg}") |
There was a problem hiding this comment.
Function may return None implicitly when results are empty and no error occurred.
When results_df.empty is True and error_msg is None, the function exits without an explicit return, implicitly returning None. This doesn't match the docstring which states it returns a Scenario. Consider whether this edge case should print a warning or return something more explicit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pysyncrosim/scenario.py` around lines 652 - 654, The function currently falls
through (implicit None) when results_df.empty is True and error_msg is None;
update the end-of-run handling so it returns an explicit Scenario object and
warns instead of returning None: when results_df.empty and error_msg is None,
emit a warning (e.g., using warnings.warn with context including self.sid and
self.name) and then return self (the Scenario) so the return type matches the
docstring; adjust the block surrounding results_df.empty and error_msg checks
(referencing results_df.empty, error_msg, self.sid, self.name, and the Scenario
return) to implement this explicit warning+return.
There was a problem hiding this comment.
this edge case where no results scenario is created after a run failure should never happen
There was a problem hiding this comment.
@vickimzhang, thanks for the clarification! If SyncroSim always produces a result scenario on a successful run, then that implicit None path is indeed unreachable and the current logic is correct. I'll note this for future reviews.
✏️ Learnings added
Learnt from: vickimzhang
Repo: syncrosim/pysyncrosim PR: 79
File: pysyncrosim/scenario.py:652-654
Timestamp: 2026-04-01T14:26:18.541Z
Learning: In `pysyncrosim/scenario.py`, the `Scenario.run()` method's `finally` block handles two branches after checking `results_df`: (1) results exist, and (2) no results with an error. The case where `results_df.empty` is `True` and `error_msg` is `None` (implicit `None` return) is intentionally unreachable — SyncroSim always creates a result scenario on a successful run, so a successful run will never produce an empty results DataFrame.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| git_repo_path = "C:/GH_ApexRMS" | ||
| lib_path = os.path.join(git_repo_path, "pysyncrosim/tests", lib_name) | ||
| lib_backup_path = os.path.join(git_repo_path, "pysyncrosim/tests", "spatial-example.ssimbak") |
There was a problem hiding this comment.
Hardcoded absolute path reduces test portability.
The path C:/GH_ApexRMS is Windows-specific and tied to a particular machine's directory structure. This will cause test failures on other developers' machines or CI environments. Consider using environment variables or a configuration file for the repository path.
🛠️ Suggested fix using environment variable
-git_repo_path = "C:/GH_ApexRMS"
+git_repo_path = os.environ.get("PYSYNCROSIM_TEST_REPO_PATH", "C:/GH_ApexRMS")Alternatively, use a path relative to the test file:
-git_repo_path = "C:/GH_ApexRMS"
+git_repo_path = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))📝 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.
| git_repo_path = "C:/GH_ApexRMS" | |
| lib_path = os.path.join(git_repo_path, "pysyncrosim/tests", lib_name) | |
| lib_backup_path = os.path.join(git_repo_path, "pysyncrosim/tests", "spatial-example.ssimbak") | |
| git_repo_path = os.environ.get("PYSYNCROSIM_TEST_REPO_PATH", "C:/GH_ApexRMS") | |
| lib_path = os.path.join(git_repo_path, "pysyncrosim/tests", lib_name) | |
| lib_backup_path = os.path.join(git_repo_path, "pysyncrosim/tests", "spatial-example.ssimbak") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_pysyncrosim.py` around lines 15 - 17, The test uses a hardcoded
Windows-only path in git_repo_path which breaks portability; replace usage in
git_repo_path, lib_path, and lib_backup_path so the repository root is
discovered dynamically (for example read from an environment variable like
REPO_ROOT via os.environ.get('REPO_ROOT') with a sensible fallback) or compute
it relative to the test file using os.path.dirname(__file__) and os.path.join to
construct lib_path and lib_backup_path; update the references in the test to use
this dynamic repo root so the tests run on different machines and CI.
|
@katieb1 let me know if I should ask @spearmangillman for a review at this point now! |
1 participant
Summary by CodeRabbit
Bug Fixes
Tests