Conversation
…as lower case in linux
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds case-insensitive environment lookups and configurable environment validation; introduces optional Mono path handling and installation validation for non‑Windows systems; converts Windows-style path joins to os.path.join; removes quoting from some CLI args; and changes setup/version loading. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Session
participant Env as Environment
participant Mono
participant Console as Console/PackageManager
Client->>Session: instantiate(location?, mono_path?)
Session->>Env: _environment() (case-insensitive lookups)
Session->>Session: _validate_environment(require_vars?)
alt non-Windows and mono required
Session->>Mono: _validate_mono(skip_if_in_environment?)
Mono-->>Session: mono path or error
end
Session->>Session: _validate_installation(location)
Session->>Console: invoke console/pkgman (prepend mono on non-Windows)
Console-->>Session: exit code / output
Session-->>Client: initialized Session or error (with context)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 1
♻️ Duplicate comments (1)
pysyncrosim/session.py (1)
419-420: Sameos.path.basename()suggestion applies here.For consistency with the earlier suggestion in
install_packages.
🧹 Nitpick comments (3)
pysyncrosim/session.py (3)
365-366: Consider usingos.path.basename()for clarity.
os.path.basename()is more idiomatic thanos.path.split()[-1]for extracting the filename.Suggested change
- if os.path.split(self.console_exe)[-1] != self.__get_console_exe_name(): + if os.path.basename(self.console_exe) != self.__get_console_exe_name():
575-576: Consider using unpacking syntax for list construction.Per static analysis suggestion,
[self.__mono_path, *final_args]is slightly more idiomatic than list concatenation.Suggested change
if not self.__is_windows: - final_args = [self.__mono_path] + final_args + final_args = [self.__mono_path, *final_args]
613-614: Same unpacking suggestion applies here.For consistency with the suggested refactor in
__call_console.Suggested change
if not self.__is_windows: - final_args = [self.__mono_path] + final_args + final_args = [self.__mono_path, *final_args]
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pysyncrosim/environment.pypysyncrosim/helper.pypysyncrosim/library.pypysyncrosim/session.pysetup.pytests/test_pysyncrosim.py
🧰 Additional context used
🧬 Code graph analysis (2)
pysyncrosim/session.py (1)
pysyncrosim/environment.py (1)
_environment(205-227)
pysyncrosim/helper.py (4)
pysyncrosim/project.py (1)
library(76-86)pysyncrosim/scenario.py (1)
library(104-114)pysyncrosim/session.py (1)
location(80-90)pysyncrosim/library.py (1)
location(125-135)
🪛 Ruff (0.14.11)
pysyncrosim/session.py
464-468: Avoid specifying long messages outside the exception class
(TRY003)
474-474: Avoid specifying long messages outside the exception class
(TRY003)
480-483: Avoid specifying long messages outside the exception class
(TRY003)
486-489: Avoid specifying long messages outside the exception class
(TRY003)
561-561: Avoid specifying long messages outside the exception class
(TRY003)
576-576: Consider [self.__mono_path, *final_args] instead of concatenation
Replace with [self.__mono_path, *final_args]
(RUF005)
578-578: subprocess call: check for execution of untrusted input
(S603)
614-614: Consider [self.__mono_path, *final_args] instead of concatenation
Replace with [self.__mono_path, *final_args]
(RUF005)
setup.py
12-12: Use of exec detected
(S102)
pysyncrosim/environment.py
247-247: Avoid specifying long messages outside the exception class
(TRY003)
257-260: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (22)
tests/test_pysyncrosim.py (1)
13-15: Good cross-platform path handling.Using
__file__withos.path.dirnameandos.path.abspathcorrectly derives the test directory at runtime, making the test portable across operating systems.pysyncrosim/library.py (2)
754-761: Good cross-platform path construction.Using
os.path.joininstead of string concatenation ensures correct path separators on both Windows and Linux.
1595-1597: LGTM.Consistent use of
os.path.joinfor the temporary export file path.pysyncrosim/helper.py (3)
56-56: Consistent argument formatting.Removing the quotes from the
--nameargument aligns with how other CLI arguments are formatted in this file (e.g., lines 142, 158). Sincesubprocesspasses arguments directly without shell interpretation, the quotes are unnecessary and could cause issues on some platforms.
234-236: LGTM.Consistent with the argument formatting pattern used elsewhere in the codebase.
266-268: LGTM.Consistent formatting for scenario deletion arguments.
setup.py (3)
8-8: Good practice: explicit UTF-8 encoding.Specifying
encoding="utf-8"ensures consistent behavior across platforms where the default encoding may differ.
10-13: Acceptable use ofexec()for version extraction.This pattern avoids importing the package during setup, which can fail if dependencies aren't installed yet. The Ruff S102 warning is a false positive since
_version.pyis a controlled file within the package, not external input.
15-29: Clean setup configuration.The restructured
setup()with explicitinstall_requiresandextras_requirefor development dependencies follows modern Python packaging practices.pysyncrosim/environment.py (7)
4-26: Good cross-platform environment variable handling.This helper correctly addresses the casing difference between Linux (lowercase) and Windows (uppercase) SyncroSim environment variables by trying all case variants.
45-48: LGTM: Validation added for data directory access.The explicit validation ensures a clear error message when the function is called outside a SyncroSim environment.
65-66: LGTM: Validation added for temp directory access.Consistent with
runtime_data_foldervalidation pattern.
99-100: Good clarifying comment.The note explains why environment validation is intentionally omitted for stdout-based progress reporting.
165-166: LGTM: Consistent with progress_bar approach.Same reasoning applies for run log updates.
207-226: LGTM: Case-insensitive environment variable lookups.All environment variables now use
_getenv_case_insensitive()for cross-platform compatibility. The default values of-1for integer fields correctly signal "not set".
229-260: Flexible environment validation.The
require_varsparameter allows functions to specify exactly which environment variables they need, providing targeted error messages. The Ruff TRY003 warnings about long exception messages are minor style preferences and don't affect functionality.pysyncrosim/session.py (6)
51-59: Good initialization order for cross-platform support.Setting
__is_windowsand__mono_pathbefore calling__init_locationis correct since the validation methods depend on these attributes. The fallback to"mono"string allows PATH lookup when no custom path is specified.
471-494: LGTM!The installation validation is thorough - checking for both required executables and providing actionable error messages with the exact paths that were checked.
501-530: Good cross-platform path discovery.The expanded search for SyncroSim installations on Linux/Mac covers common locations and provides a helpful error message listing all searched paths with clear solutions. Validating that the console executable exists before selecting a path prevents false positives from empty directories.
540-548: LGTM!Good encapsulation of executable names. The comments correctly note that Mono runs
.exefiles on Linux, so the same names are used across platforms.
583-597: LGTM!The enhanced error context for path-related errors is helpful for debugging cross-platform issues. Including the location, console executable path, and mono command makes troubleshooting much easier.
635-643: LGTM!Using the
inoperator for string containment is idiomatic Python and the logic correctly handles the "No conda configuration yet" case.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pysyncrosim/session.py`:
- Around line 479-509: There are two identical definitions of the method restore
in the class (duplicate copy-paste) causing F811; remove the duplicate so only a
single restore method remains. Locate both def restore(...) definitions (the
ones that validate filepath and optional folder, build args with "--restore" and
"--lib", call self.__call_console and print result.stdout) and delete one of
them so the class has only one restore implementation; ensure any references to
__call_console, args, and the optional folder handling remain intact in the kept
method.
♻️ Duplicate comments (1)
pysyncrosim/session.py (1)
511-527: Validation ignores user-providedmono_path.When a user specifies a custom
mono_path(e.g.,Session(mono_path="/usr/local/bin/mono")), this method still validates usingshutil.which("mono")on the system PATH, which may fail even though the user's custom path is valid. The method should validateself.__mono_pathwhen it differs from the default.🐛 Proposed fix to validate user-provided path
def _validate_mono(self, skip_if_in_environment=False): """Validate Mono is available on Linux systems""" # Skip validation if running inside a SyncroSim environment if skip_if_in_environment: e = ps.environment._environment() if e.program_directory.item() is not None: # Running inside SyncroSim environment, skip validation - return "mono" + return self.__mono_path + + # If user provided custom path, validate it exists + if self.__mono_path != "mono": + if shutil.which(self.__mono_path) or os.path.isfile(self.__mono_path): + return self.__mono_path + raise RuntimeError( + f"Specified mono path not found: {self.__mono_path}\n" + "Verify the path exists and is executable." + ) - mono_path = shutil.which("mono") - if mono_path is None: + # Default case: search for mono in PATH + found_mono = shutil.which("mono") + if found_mono is None: raise RuntimeError( "Mono is required to run SyncroSim on Linux but was not found.\n" "Install Mono: sudo apt-get install mono-complete\n" "Or specify custom path: Session(location=..., mono_path=...)" ) - return mono_path + return found_mono
🧹 Nitpick comments (1)
pysyncrosim/session.py (1)
633-655: Good Mono integration with enhanced error context.The Mono path prepending and enhanced error messages with troubleshooting information will help users diagnose issues on Linux.
Consider the more idiomatic list construction as suggested by static analysis:
♻️ Optional: Use spread operator for list construction
if not self.__is_windows: - final_args = [self.__mono_path] + final_args + final_args = [self.__mono_path, *final_args]
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pysyncrosim/helper.pypysyncrosim/library.pypysyncrosim/session.py
🚧 Files skipped from review as they are similar to previous changes (1)
- pysyncrosim/library.py
🧰 Additional context used
🧬 Code graph analysis (2)
pysyncrosim/helper.py (4)
pysyncrosim/project.py (1)
library(76-86)pysyncrosim/scenario.py (1)
library(104-114)pysyncrosim/session.py (1)
location(80-90)pysyncrosim/library.py (1)
location(124-134)
pysyncrosim/session.py (2)
pysyncrosim/library.py (2)
location(124-134)environment(137-147)pysyncrosim/environment.py (1)
_environment(205-227)
🪛 Ruff (0.14.11)
pysyncrosim/session.py
479-479: Redefinition of unused restore from line 447: restore redefined here
(F811)
499-499: Avoid specifying long messages outside the exception class
(TRY003)
505-505: Avoid specifying long messages outside the exception class
(TRY003)
522-526: Avoid specifying long messages outside the exception class
(TRY003)
532-532: Avoid specifying long messages outside the exception class
(TRY003)
538-541: Avoid specifying long messages outside the exception class
(TRY003)
544-547: Avoid specifying long messages outside the exception class
(TRY003)
619-619: Avoid specifying long messages outside the exception class
(TRY003)
634-634: Consider [self.__mono_path, *final_args] instead of concatenation
Replace with [self.__mono_path, *final_args]
(RUF005)
636-636: subprocess call: check for execution of untrusted input
(S603)
672-672: Consider [self.__mono_path, *final_args] instead of concatenation
Replace with [self.__mono_path, *final_args]
(RUF005)
🔇 Additional comments (12)
pysyncrosim/helper.py (3)
56-56: LGTM - Correct CLI argument formatting.Removing quotes from
--name=%sis appropriate sincesubprocess.run()passes arguments directly without shell interpretation. Explicit quotes would be incorrectly included in the argument value itself, which could cause issues on Linux with Mono.
240-242: LGTM - Consistent CLI argument formatting.The unquoted
--lib=%sformat is consistent with the library creation changes and correct for subprocess argument passing.
272-274: LGTM - Consistent CLI argument formatting.The unquoted
--lib=%sformat is consistent with the other changes in this file.pysyncrosim/session.py (9)
16-53: Well-structured initialization with platform detection.Good design choice to set
__is_windowsand__mono_pathearly in the initialization sequence before other methods that depend on them are called. Themono_pathparameter is properly documented with usage examples.
529-553: LGTM - Installation validation is thorough.The validation method correctly checks for the installation directory, required executables, and Mono availability on non-Windows systems. The skip logic for SyncroSim environments is appropriate.
554-596: Good Linux path discovery with helpful error messages.The expanded path search for Linux installations and the detailed error message listing searched paths will significantly improve the user experience on non-Windows systems.
598-607: LGTM - Good encapsulation of executable names.These helper methods provide a clean abstraction for executable names, making future platform-specific changes easier. The comments correctly explain that Mono uses .exe files on Linux.
608-619: LGTM - Clean refactor using helper methods.The console initialization now uses the encapsulated executable name helpers, improving maintainability.
671-684: LGTM - Consistent Mono integration for interactive console.The interactive console correctly handles Mono prepending and maintains different subprocess configurations for Windows vs non-Windows platforms.
357-360: LGTM - Proper use of helper method for exe comparison.The check now correctly uses
__get_console_exe_name()to verify the current executable, ensuring the console is only reset when necessary.
411-414: LGTM - Consistent exe check pattern.Same pattern as
install_packages, maintaining consistency.
693-702: LGTM - Standard string containment check.Using
infor string containment is idiomatic Python.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@pysyncrosim/environment.py`:
- Around line 206-237: The _validate_environment function should treat unknown
variable names and blank-string values as missing: update the require_vars loop
in _validate_environment to append var to missing if var not in e.columns, and
when present treat values that are None, empty string (after strip()), or the
sentinel int -1 as missing; keep referencing _validate_environment and the
_environment() DataFrame to locate the fix and raise the same RuntimeError with
the collected missing names.
In `@pysyncrosim/session.py`:
- Around line 671-679: The method __retrieve_conda_filepath currently calls
__call_console which raises on non‑zero return codes; wrap the __call_console
invocation in a try/except to catch the exception (from __call_console) and
inspect the exception's stderr (decoded) so you can detect the "No conda
configuration yet" message and set self.__conda_filepath = None, otherwise
extract the filepath as before (cleaned_result.split(": ")[1]) or re‑raise the
exception for unexpected errors; update __retrieve_conda_filepath to use this
try/except behavior referencing __call_console and self.__conda_filepath.
- Around line 541-556: The auto-discovery currently selects the first directory
in possible_paths that contains the console executable (checked via
__get_console_exe_name()) which can yield a partial install and later cause
_validate_installation to fail; update the loop that sets location so it only
accepts a candidate directory if both the console executable and the
package-manager executable (use the package manager helper—e.g.,
__get_package_mgr_exe_name() or equivalent) exist in that directory before
assigning location and breaking, ensuring _validate_installation will not be
triggered by a partial install.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pysyncrosim/session.py`:
- Around line 489-505: The block that checks a custom mono path around
self.__mono_path uses inconsistent indentation (mix of 2 and 4 spaces) causing
an IndentationError; fix by normalizing indentation to the project's Python
style (use 4 spaces) for the entire if self.__mono_path != "mono": block
including the inner if (shutil.which(self.__mono_path) or
os.path.isfile(self.__mono_path)), the subsequent return self.__mono_path, and
the raise RuntimeError message so the control flow is consistent and the
function (the Session mono-path resolution logic) parses correctly.
🧹 Nitpick comments (2)
pysyncrosim/session.py (2)
549-555: Consider checking both executables in auto-discovery for efficiency.The discovery loop only checks for the console executable before selecting a candidate. While
_validate_installationwill catch partial installs downstream, checking both executables here would avoid attempting to validate directories that will inevitably fail.♻️ Suggested improvement
for path in possible_paths: if os.path.isdir(path): # Check if executables exist in this path console_path = os.path.join(path, self.__get_console_exe_name()) - if os.path.isfile(console_path): + pkgman_path = os.path.join(path, self.__get_pkgman_exe_name()) + if os.path.isfile(console_path) and os.path.isfile(pkgman_path): location = path break
606-612: Consider using list unpacking for cleaner syntax.Static analysis suggests using
[self.__mono_path, *final_args]instead of list concatenation. This is a minor style preference that's more idiomatic in modern Python.♻️ Suggested improvement
if not self.__is_windows: - final_args = [self.__mono_path] + final_args + final_args = [self.__mono_path, *final_args]
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pysyncrosim/session.py (2)
549-555:⚠️ Potential issue | 🟠 MajorAuto-discovery still picks partial installs.
Line 553 only checks for the console executable before selecting
location. This can still stop on a partial install and fail later, even when a later candidate is valid.Proposed fix
- for path in possible_paths: - if os.path.isdir(path): - # Check if executables exist in this path - console_path = os.path.join(path, self.__get_console_exe_name()) - if os.path.isfile(console_path): - location = path - break + for path in possible_paths: + if os.path.isdir(path): + console_path = os.path.join(path, self.__get_console_exe_name()) + pkgman_path = os.path.join(path, self.__get_pkgman_exe_name()) + if os.path.isfile(console_path) and os.path.isfile(pkgman_path): + location = path + break🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pysyncrosim/session.py` around lines 549 - 555, The auto-discovery loop currently sets location as soon as the console exe exists, which can pick a partial install; instead verify all required executables/files exist before assigning location by checking both the console executable (self.__get_console_exe_name()) and the core/runtime executable or other required files (e.g., self.__get_core_exe_name() or any other expected filenames) with os.path.isfile on the candidate path; only set location and break when every required file is present, otherwise continue to the next entry in possible_paths.
666-674:⚠️ Potential issue | 🟠 Major
__retrieve_conda_filepathno longer handles non-zero responses correctly.Line 667 calls
__call_console, which now raises on non-zero return codes. Theif result.returncode != 0branch is effectively dead and the"No conda configuration yet"case can become a hard failure.Proposed fix
def __retrieve_conda_filepath(self): - result = self.__call_console(["--conda", "--config"]) - - if result.returncode != 0: - cleaned_result = result.stderr.decode('utf-8').strip() - if "No conda configuration yet" in cleaned_result: - self.__conda_filepath = None - else: - self.__conda_filepath = cleaned_result.split(": ")[1] + try: + result = self.__call_console(["--conda", "--config"]) + except RuntimeError as e: + cleaned_result = str(e).strip() + if "No conda configuration yet" in cleaned_result: + self.__conda_filepath = None + return + raise + + cleaned_result = ( + result.stderr.decode('utf-8').strip() + or result.stdout.decode('utf-8').strip() + ) + if cleaned_result.startswith("Conda path: "): + self.__conda_filepath = cleaned_result.split(": ", 1)[1]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pysyncrosim/session.py` around lines 666 - 674, The __retrieve_conda_filepath function assumes __call_console returns a result object but __call_console now raises on non-zero exit codes, so change the implementation to call __call_console in a try/except: on success parse the normal result (e.g., result.stdout or result) to set self.__conda_filepath, and in the except block catch the raised error (the exception from __call_console), decode/inspect its stderr or message and if it contains "No conda configuration yet" set self.__conda_filepath = None, otherwise extract the path portion (split on ": ") and assign it; reference __retrieve_conda_filepath, __call_console, and self.__conda_filepath when updating the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pysyncrosim/session.py`:
- Around line 549-555: The auto-discovery loop currently sets location as soon
as the console exe exists, which can pick a partial install; instead verify all
required executables/files exist before assigning location by checking both the
console executable (self.__get_console_exe_name()) and the core/runtime
executable or other required files (e.g., self.__get_core_exe_name() or any
other expected filenames) with os.path.isfile on the candidate path; only set
location and break when every required file is present, otherwise continue to
the next entry in possible_paths.
- Around line 666-674: The __retrieve_conda_filepath function assumes
__call_console returns a result object but __call_console now raises on non-zero
exit codes, so change the implementation to call __call_console in a try/except:
on success parse the normal result (e.g., result.stdout or result) to set
self.__conda_filepath, and in the except block catch the raised error (the
exception from __call_console), decode/inspect its stderr or message and if it
contains "No conda configuration yet" set self.__conda_filepath = None,
otherwise extract the path portion (split on ": ") and assign it; reference
__retrieve_conda_filepath, __call_console, and self.__conda_filepath when
updating the logic.
2 participants
Summary by CodeRabbit
New Features
Improvements
Chores