Conversation
📝 WalkthroughWalkthroughAdds folder- and datasheet-scoped deletion: two new helpers ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Library
participant Project
participant Scenario
participant Helper
participant Console
User->>Library: delete(folder=fid, force=True)
Library->>Helper: _delete_folder(library, fid, force=True)
Helper->>Console: execute delete --folder fid --library <location>
Console-->>Helper: success
Helper-->>Library: complete
Library-->>User: return location
User->>Project: delete(datasheet="X", ids=[1,2], force=True)
Project->>Library: delete(datasheet="X", ids=[1,2], pid=<pid>, force=True)
Library->>Helper: _delete_data(library, "X", pid=<pid>, ids=[1,2], force=True)
Helper->>Console: execute delete --data --datasheet X --pid <pid> --ids 1,2 --library <location>
Console-->>Helper: success
Helper-->>Library: complete
Library-->>Project: done
Project-->>User: return location
User->>Scenario: delete(datasheet="Y", ids=[3], force=True)
Scenario->>Library: delete(datasheet="Y", ids=[3], sid=<sid>, force=True)
Library->>Helper: _delete_data(library, "Y", sid=<sid>, ids=[3], force=True)
Helper->>Console: execute delete --data --datasheet Y --sid <sid> --ids 3 --library <location>
Console-->>Helper: success
Helper-->>Library: complete
Library-->>Scenario: done
Scenario-->>User: return location
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
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✏️ Tip: You can disable this entire section by setting 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pysyncrosim/scenario.py (1)
516-547: Theforceparameter is ignored when deleting the Scenario itself.When
datais notTrue, the method hardcodesforce=Trueat line 546, which means the user'sforceargument is ignored for Scenario deletions. This behavior differs from the docstring which states the parameter controls whether the user is prompted for confirmation.If this is intentional (to always force Scenario deletions without prompts), consider either:
- Documenting this behavior in the docstring, or
- Using the passed
forceparameter for consistencyProposed fix to respect the force parameter
else: self.library.delete(project=self.project, scenario=self, - force=True) + force=force)pysyncrosim/library.py (2)
679-732: Missing implementation: Data deletion branch not implemented.The method signature includes
data,datasheet,pid,sid, andidsparameters, andhelper._delete_data()exists, but there's no code path that handles data deletion. The tests intest_delete_datasheetexpect this to work.You need to add a branch to handle data deletion before or after the existing project/scenario/folder branches:
Proposed implementation for data deletion
+ if not isinstance(data, bool) and data is not None: + raise TypeError("data must be a Logical") + if datasheet is not None and not isinstance(datasheet, str): + raise TypeError("datasheet must be a String") + if pid is not None and not isinstance(pid, int) and not isinstance(pid, np.int64): + raise TypeError("pid must be an Integer") + if sid is not None and not isinstance(sid, int) and not isinstance(sid, np.int64): + raise TypeError("sid must be an Integer") + + if data is True: + if datasheet is None: + raise ValueError("datasheet name is required") + helper._delete_data(library=self, datasheet=datasheet, + pid=pid, sid=sid, ids=ids, + session=self.session, force=force) + return + if project is None and scenario is None and folder is None:
914-933: Unreachable code incompactmethod.The
elseblock at lines 932-933 is dead code because:
- If the
tryblock succeeds, it returns at line 926- If an exception occurs, the
exceptblock raises at line 929-930The
elseclause in a try/except/else structure only runs if no exception was raised AND no return/break/continue occurred in try - but line 926 returns before reaching else.Proposed fix to remove dead code
def compact(self): """ Compacts a SyncroSim library to reduce its footprint on disk. Returns ------- String. Path to the compacted library. """ try: args = ["--compact", f"--lib={self.location}"] self.session._Session__call_console(args) return self.location except RuntimeError as e: raise RuntimeError(f"Failed to compact library with the following " f"error message: {e}.") - - else: - return self.location
🤖 Fix all issues with AI agents
In @pysyncrosim/library.py:
- Around line 616-619: The delete method signature is missing the folder
parameter used in the implementation and contains unused params (pid, sid, data,
datasheet, ids); update the pysyncrosim.library.Library.delete signature to
include folder (e.g., folder=None) and either remove or wire up the unused
parameters: implement the data/datasheet deletion branch (add an elif data is
True: / elif datasheet is True: path that calls helper._delete_data(...) or
appropriate helper methods using pid, sid, ids) and remove
pid/sid/data/datasheet/ids if you decide not to support them; ensure all
references to folder in the body match the new parameter name and that
helper._delete_data is called where data/datasheet deletion is intended.
In @pysyncrosim/project.py:
- Around line 300-332: The Project.delete method is passing the Project instance
as scenario (scenario=self) and also ignores the method's scenario parameter;
update Project.delete so that in the non-data branch it forwards the actual
scenario argument to self.library.delete (i.e., call
self.library.delete(project=self, scenario=scenario, force=force) instead of
scenario=self), and before calling resolve the scenario param when provided (if
it's an ID/string, convert it to the proper Scenario object or ID using the
existing project-level resolver/helper you use elsewhere) so library.delete
receives a correct Scenario or None; also stop hardcoding force=True and forward
the force argument.
In @tests/test_pysyncrosim.py:
- Around line 363-375: Define the missing folder objects before deletion: create
a folder with the library API (e.g., call the folder-creation helper used
elsewhere such as myLibrary.create_folder(...) or myLibrary.create(...)) and
assign it to myFolder, then set fid = myFolder.folder_id (or extract the ID
returned by the creation call). This ensures myFolder and fid exist before
calling myLibrary.delete(folder=myFolder, force=True) and
myLibrary.delete(folder=fid, force=True), and that subsequent assertions against
myLibrary.folders() are valid.
- Around line 442-444: The f-string constructing ids_to_delete uses nested
double quotes causing a Python 3.11 SyntaxError; update the f-string to use
single quotes for the inner key lookup on saved_data (referencing the
ids_to_delete assignment that reads saved_data.iloc[0]["StratumId"] and
saved_data.iloc[1]["StratumId"]) so it becomes valid in Python 3.11, leaving the
subsequent myLibrary.delete call (datasheet="stsim_Stratum", pid=myProject.pid,
ids=ids_to_delete, force=True) unchanged.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pysyncrosim/helper.pypysyncrosim/library.pypysyncrosim/project.pypysyncrosim/scenario.pytests/test_pysyncrosim.py
🧰 Additional context used
🧬 Code graph analysis (3)
pysyncrosim/scenario.py (2)
pysyncrosim/library.py (1)
delete(616-732)pysyncrosim/project.py (2)
delete(300-332)library(76-86)
tests/test_pysyncrosim.py (3)
pysyncrosim/library.py (6)
delete(616-732)folders(513-523)projects(371-431)name(105-115)name(118-121)datasheets(526-614)pysyncrosim/project.py (7)
delete(300-332)folders(221-251)name(56-66)name(69-73)library(76-86)pid(43-53)datasheets(253-298)pysyncrosim/scenario.py (8)
delete(516-547)folders(334-357)project(91-101)name(65-80)name(83-88)library(104-114)sid(52-62)datasheets(359-404)
pysyncrosim/project.py (2)
pysyncrosim/library.py (1)
delete(616-732)pysyncrosim/scenario.py (3)
delete(516-547)library(104-114)project(91-101)
🪛 Ruff (0.14.11)
pysyncrosim/library.py
617-617: Unused method argument: pid
(ARG002)
617-617: Unused method argument: sid
(ARG002)
617-617: Unused method argument: data
(ARG002)
617-617: Unused method argument: datasheet
(ARG002)
617-617: Unused method argument: ids
(ARG002)
679-679: Undefined name folder
(F821)
721-721: Undefined name folder
(F821)
725-725: Undefined name folder
(F821)
726-726: Undefined name folder
(F821)
727-727: Undefined name folder
(F821)
728-728: Undefined name folder
(F821)
730-730: Avoid specifying long messages outside the exception class
(TRY003)
pysyncrosim/helper.py
298-298: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_pysyncrosim.py
371-371: Undefined name myFolder
(F821)
372-372: Undefined name myFolder
(F821)
374-374: Undefined name fid
(F821)
375-375: Undefined name fid
(F821)
pysyncrosim/project.py
300-300: Unused method argument: scenario
(ARG002)
🔇 Additional comments (2)
pysyncrosim/helper.py (2)
280-303: LGTM - _delete_folder implementation is well-structured.The function properly:
- Initializes session if not provided
- Prompts for confirmation unless
force=True- Validates folder existence before deletion (lines 292-298)
- Raises a clear
ValueErrorif the folder doesn't exist
305-331: LGTM - _delete_data implementation follows existing patterns.The function correctly handles optional
pid,sid, andidsparameters, and delegates validation to the SyncroSim console. This is consistent with the existingsave_datasheetapproach.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pysyncrosim/library.py (1)
963-973: Unreachableelseblock creates dead code.The
tryblock now returnsself.locationat line 966. Since theexceptblock raises an exception, theelseblock at lines 972-973 is never reached. This is likely a leftover from adding the return statement.Suggested fix - remove the dead code
def compact(self): """ Compacts a SyncroSim library to reduce its footprint on disk. Returns ------- String. Path to the compacted library. """ try: args = ["--compact", f"--lib={self.location}"] self.session._Session__call_console(args) return self.location except RuntimeError as e: raise RuntimeError(f"Failed to compact library with the following " f"error message: {e}.") - - else: - return self.location
🤖 Fix all issues with AI agents
In @pysyncrosim/library.py:
- Around line 758-772: The deletion branch currently only accepts Python int and
ps.Folder, causing np.int64 to hit the TypeError; update the type check to
accept numpy integer types (or any integral) and coerce to a Python int before
calling helper._delete_folder. Replace the "if type(folder) is int:" check with
an isinstance check such as isinstance(folder, (int, np.integer)) or import
numbers and use isinstance(folder, numbers.Integral), then set fid =
int(folder); leave the ps.Folder branch and the helper._delete_folder call
unchanged.
In @pysyncrosim/project.py:
- Around line 327-336: The project deletion branch currently calls
self.library.delete(project=self, force=True) which ignores the caller's force
argument; update that branch to pass the provided force parameter (i.e., use
force=force) so the method respects project.delete(force=False) and other calls
that rely on the passed-in flag; keep the other branches unchanged.
🧹 Nitpick comments (3)
pysyncrosim/library.py (2)
682-696: Missing type validation foridsparameter.The
idsparameter is validated in other new parameters but not here. Based on the docstring,idsshould be a String. Consider adding type validation for consistency.Suggested fix
if sid is not None and not isinstance(sid, int)\ and not isinstance(sid, np.int64): raise TypeError("sid must be an Integer") + if ids is not None and not isinstance(ids, str): + raise TypeError("ids must be a String")
707-713: Consider adding explicit return after data deletion.After
_delete_datacompletes, the method falls through and returnsNoneimplicitly. For clarity and consistency with the library deletion case (which also doesn't return anything), an explicitreturnwould make the flow clearer.Suggested improvement
if data is True: if datasheet is None: raise ValueError("datasheet name is required") helper._delete_data(library=self, datasheet=datasheet, pid=pid, sid=sid, ids=ids, session=self.session, force=force) + returnpysyncrosim/project.py (1)
300-336: Consider adding type validation for consistency.Unlike
Library.delete, this method doesn't validate the types ofdata,datasheet, andidsparameters. WhileLibrary.deletehandles validation downstream, adding checks here would provide earlier error messages with clearer context.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pysyncrosim/library.pypysyncrosim/project.pytests/test_pysyncrosim.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_pysyncrosim.py
🧰 Additional context used
🧬 Code graph analysis (2)
pysyncrosim/project.py (2)
pysyncrosim/library.py (1)
delete(616-772)pysyncrosim/scenario.py (3)
delete(516-547)library(104-114)project(91-101)
pysyncrosim/library.py (2)
pysyncrosim/folder.py (3)
Folder(8-271)name(80-93)name(96-99)pysyncrosim/helper.py (4)
_delete_data(305-331)library(6-83)_delete_library(164-211)_delete_folder(280-303)
🪛 Ruff (0.14.11)
pysyncrosim/library.py
685-685: Avoid specifying long messages outside the exception class
(TRY003)
688-688: Avoid specifying long messages outside the exception class
(TRY003)
690-690: Avoid specifying long messages outside the exception class
(TRY003)
693-693: Avoid specifying long messages outside the exception class
(TRY003)
696-696: Avoid specifying long messages outside the exception class
(TRY003)
710-710: Avoid specifying long messages outside the exception class
(TRY003)
769-769: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (3)
pysyncrosim/library.py (2)
616-619: LGTM on the expanded delete signature.The new parameters (
folder,pid,sid,data,datasheet,ids) extend the delete functionality appropriately for folder-based and datasheet data deletions.
715-720: LGTM on the library deletion branch.The condition correctly checks that no specific entity (project, scenario, folder) is specified before defaulting to library deletion. The new removal flags are properly passed to the helper function.
pysyncrosim/project.py (1)
300-301: LGTM on the expanded delete signature.The new parameters align with the Library.delete signature for data deletion operations at project scope.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pysyncrosim/library.py (3)
726-734: Sequentialifstatements could cause issues with np.int64.The pattern uses
type(project) is intwhich won't matchnp.int64, but the earlier type check at line 674 allowsnp.int64. Ifprojectisnp.int64, none of theifbranches will execute, leavingpundefined.Consider using
elifchain orisinstance:- if type(project) is int: + if isinstance(project, (int, np.int64)): p = self.projects(pid = project) - if type(project) is str: + elif isinstance(project, str): if project in self.__projects["Name"].values: p = self.projects(name = project) else: raise ValueError(f'project {project} does not exist') - if isinstance(project, ps.Project): + elif isinstance(project, ps.Project): p = project + else: + raise TypeError("project must be a Project instance, Integer, or String")
744-752: Samenp.int64issue in scenario deletion branch.The same sequential
ifpattern issue exists here. Ifscenarioisnp.int64, none of the branches will execute properly.
963-973: Unreachable code afterreturnstatement.The
elseblock at lines 972-973 is unreachable because line 966 now returns before it. Theelsebelongs to thetryblock and only executes if no exception occurs, but thereturnat line 966 already handles that case.try: args = ["--compact", f"--lib={self.location}"] self.session._Session__call_console(args) return self.location except RuntimeError as e: raise RuntimeError(f"Failed to compact library with the following " f"error message: {e}.") - - else: - return self.location
🧹 Nitpick comments (2)
pysyncrosim/project.py (1)
300-336: Missing type validation for new parameters.The
deletemethod delegates tolibrary.delete()which performs type checks, but adding local validation would provide clearer error messages scoped to the Project context. TheScenario.delete()method (lines 515-546 in scenario.py) similarly lacks local validation but relies on library-level checks.Also, when
scenariois passed without aprojectreference (line 332), the library's delete might have difficulty resolving the scenario if there are multiple projects. Consider passingproject=selfto ensure proper context:elif scenario is not None: - self.library.delete(scenario = scenario, force = force) + self.library.delete(project=self, scenario=scenario, force=force)tests/test_pysyncrosim.py (1)
446-446: Inconsistent quote style in f-string may cause issues in some Python versions.The f-string uses mixed quote styles which could be cleaner:
- ids_to_delete = f"{saved_data.iloc[0]['StratumId']},{saved_data.iloc[1]["StratumId"]}" + ids_to_delete = f"{saved_data.iloc[0]['StratumId']},{saved_data.iloc[1]['StratumId']}"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pysyncrosim/library.pypysyncrosim/project.pytests/test_pysyncrosim.py
🧰 Additional context used
🧬 Code graph analysis (3)
pysyncrosim/project.py (2)
pysyncrosim/library.py (1)
delete(616-772)pysyncrosim/scenario.py (3)
delete(516-547)library(104-114)project(91-101)
pysyncrosim/library.py (4)
pysyncrosim/project.py (5)
delete(300-335)pid(43-53)library(76-86)name(56-66)name(69-73)pysyncrosim/scenario.py (6)
delete(516-547)project(91-101)sid(52-62)library(104-114)name(65-80)name(83-88)pysyncrosim/helper.py (4)
_delete_data(305-331)library(6-83)_delete_library(164-211)_delete_folder(280-303)pysyncrosim/session.py (1)
location(72-82)
tests/test_pysyncrosim.py (1)
pysyncrosim/library.py (7)
delete(616-772)projects(371-431)name(105-115)name(118-121)folders(513-523)scenarios(434-510)session(92-102)
🪛 Ruff (0.14.11)
pysyncrosim/library.py
685-685: Avoid specifying long messages outside the exception class
(TRY003)
688-688: Avoid specifying long messages outside the exception class
(TRY003)
690-690: Avoid specifying long messages outside the exception class
(TRY003)
693-693: Avoid specifying long messages outside the exception class
(TRY003)
696-696: Avoid specifying long messages outside the exception class
(TRY003)
710-710: Avoid specifying long messages outside the exception class
(TRY003)
769-769: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (6)
tests/test_pysyncrosim.py (2)
363-379: LGTM! Good coverage for folder deletion.The tests properly validate:
- TypeError for invalid folder argument
- ValueError for non-existent folder ID
- Deletion by Folder object and by folder ID
- Verification that deleted folders are removed from
Library.folders()
391-451: Good test coverage for datasheet deletion.The test function comprehensively covers:
- Type validation for
data,datasheet,pid,sidparameters- Required field validation (
datasheet name is required)- Deletion from Project scope via Library class
- Deletion from Scenario scope via Library class
- Deletion via Project and Scenario class interfaces
- Row ID-based partial deletion with verification
pysyncrosim/library.py (4)
616-706: Type validation is comprehensive.The new type checks for
folder,data,datasheet,pid, andsidparameters are well-structured and consistent with the existing validation pattern. The static analysis hints about long exception messages (TRY003) are minor style concerns that don't impact functionality.
707-713: LGTM! Datasheet deletion logic is correct.The branch properly validates that
datasheetis required whendata=Trueand delegates to the helper function with all necessary parameters.
715-720: Verify intent:data=Falsefalls through to library deletion.The condition
project is None and scenario is None and folder is Nonedoesn't account fordata=False. If a user explicitly passesdata=Falsewith no other parameters, it will delete the entire library rather than being a no-op.This is likely intentional since
data=Falseis semantically "don't delete data" (not "delete data=False"), but worth confirming the expected behavior.
759-772: LGTM! Folder deletion logic is correct.The folder deletion branch properly handles both
int/np.int64andFolderinstance cases. The redundant type check at line 768-769 provides defense in depth, which is acceptable.
katieb1
left a comment
katieb1
left a comment
There was a problem hiding this comment.
Good first pass! I think we can pare down the number of arguments for each of these functions to make them a bit more streamlined.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pysyncrosim/library.py (1)
939-949: Unreachable code in compact method.Line 942 returns
self.locationon success, making theelseblock at lines 948-949 unreachable. Theelseclause of a try-except only executes if no exception was raised, but the function already returns before reaching it.Proposed fix
def compact(self): ... try: args = ["--compact", f"--lib={self.location}"] self.session._Session__call_console(args) return self.location except RuntimeError as e: raise RuntimeError(f"Failed to compact library with the following " f"error message: {e}.") - - else: - return self.location
🤖 Fix all issues with AI agents
In `@pysyncrosim/project.py`:
- Around line 309-314: The docstring for the parameter "datasheet" incorrectly
refers to a non-existent "data" parameter; update the docstring in
pysyncrosim/project.py (the method that defines the parameters datasheet and
ids) to either remove the phrase "Required when data is True" or replace it with
the correct condition/name (e.g., "Required when delete_data is True" if such a
flag exists); ensure the "datasheet" description and the "ids" description
accurately reflect the actual method signature and behavior.
- Around line 324-325: The datasheet deletion call in the Project class is
missing the project scope: update the call to self.library.delete(...) inside
the Project.delete/_delete_data flow to include pid=self.pid (i.e., pass
pid=self.pid along with datasheet, ids, force) so deletions are scoped to the
current Project (mirror how save_datasheet() passes self.pid); ensure any helper
_delete_data continues to forward pid and sid when provided.
In `@pysyncrosim/scenario.py`:
- Around line 541-543: In the Scenario.delete path where datasheet is None
you’re always calling self.library.delete(..., force=True) which ignores the
user-provided force argument; change that call to forward the method parameter
(use force rather than True) so the else branch mirrors Project.delete and
respects the caller’s force flag (locate the delete method on class Scenario and
update the self.library.delete(project=self.project, scenario=self, force=...)
invocation).
- Around line 522-527: The docstring for the datasheet parameter incorrectly
references a non-existent "data" parameter; update the docstring for datasheet
(and optionally ids) to remove or replace "Required when data is True" with a
correct description such as "Required when deleting rows from a specific
datasheet" or simply "Required when deleting data" so the docstring matches the
current method signature and parameters (datasheet, ids).
In `@tests/test_pysyncrosim.py`:
- Around line 413-426: Tests reference non-existent kwargs (data, pid, sid) for
Library.delete; update the tests to use the actual parameter names from
delete(self, project=None, scenario=None, folder=None, datasheet=None, ids=None,
...): replace data with ids, pid with project, and sid with scenario in the
failing calls (e.g., change myLibrary.delete(data="True") ->
myLibrary.delete(ids="True"), change myLibrary.delete(data=True,
datasheet="core_Backup", pid="1") -> myLibrary.delete(ids=True,
datasheet="core_Backup", project="1"), and change myLibrary.delete(..., sid="1")
-> myLibrary.delete(..., scenario="1")), and update the expected TypeError match
strings to reference the correct parameter names ("ids must be ...", "project
must be an Integer", "scenario must be an Integer") while leaving the
datasheet-required ValueError assertion as-is.
- Around line 440-447: The tests call Project.delete and Scenario.delete with a
non-existent data=True parameter; remove the data=True argument so calls use the
correct signature (e.g., myProject.delete(datasheet="stsim_Stratum", force=True)
and myScenario2.delete(datasheet="stsim_RunControl", force=True)); update the
other occurrences noted in the review (lines referenced around the same test:
the calls at the earlier positions mentioned) to drop data=True as well so all
delete invocations use only the valid datasheet and force parameters.
♻️ Duplicate comments (2)
tests/test_pysyncrosim.py (1)
453-453: Fix inconsistent quote usage in f-string.Line 453 has mixed quote styles:
saved_data.iloc[0]['StratumId']uses single quotes butsaved_data.iloc[1]["StratumId"]uses double quotes. The double quotes inside the double-quoted f-string will cause aSyntaxErrorin Python 3.11.Proposed fix
- ids_to_delete = f"{saved_data.iloc[0]['StratumId']},{saved_data.iloc[1]["StratumId"]}" + ids_to_delete = f"{saved_data.iloc[0]['StratumId']},{saved_data.iloc[1]['StratumId']}"pysyncrosim/library.py (1)
676-679: Datasheet deletion lacks scope parameters.The
deletemethod callshelper._delete_datawithoutpidorsidparameters, even though the helper supports them. WhenProject.deleteorScenario.deletedelegates datasheet deletion here, there's no way to pass the scope. Consider addingpidandsidparameters to the method signature for proper scoping.Proposed fix
Update the method signature and implementation:
def delete(self, project=None, scenario=None, folder=None, - datasheet=None, ids=None, force=False, remove_backup=False, remove_publish=False, remove_custom_folders=False): + datasheet=None, pid=None, sid=None, ids=None, force=False, remove_backup=False, remove_publish=False, remove_custom_folders=False): ... # delete datasheet if datasheet is not None: - helper._delete_data(library=self, datasheet=datasheet, ids=ids, + helper._delete_data(library=self, datasheet=datasheet, pid=pid, sid=sid, ids=ids, session=self.session, force=force)Then update
Project.deleteto passpid=self.pidandScenario.deleteto passsid=self.sid.
🧹 Nitpick comments (1)
pysyncrosim/library.py (1)
706-707: Remove extraneous f-prefix from string literals.These f-strings don't contain any placeholders, so the
fprefix is unnecessary.Proposed fix
- raise TypeError(f"project must be a Project instance, " - f"Integer, or String") + raise TypeError("project must be a Project instance, " + "Integer, or String")Same fix applies to lines 729-730.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pysyncrosim/helper.pypysyncrosim/library.pypysyncrosim/project.pypysyncrosim/scenario.pytests/test_pysyncrosim.py
🧰 Additional context used
🧬 Code graph analysis (4)
pysyncrosim/scenario.py (2)
pysyncrosim/library.py (1)
delete(616-748)pysyncrosim/project.py (2)
delete(300-331)library(76-86)
tests/test_pysyncrosim.py (1)
pysyncrosim/library.py (7)
delete(616-748)projects(371-431)name(105-115)name(118-121)folders(513-523)session(92-102)scenarios(434-510)
pysyncrosim/library.py (1)
pysyncrosim/helper.py (6)
_delete_data(305-331)library(6-83)_delete_library(164-211)_delete_project(213-246)_delete_scenario(248-278)_delete_folder(280-303)
pysyncrosim/project.py (2)
pysyncrosim/library.py (1)
delete(616-748)pysyncrosim/scenario.py (3)
delete(516-543)library(104-114)project(91-101)
🪛 Ruff (0.14.11)
pysyncrosim/library.py
663-663: Avoid specifying long messages outside the exception class
(TRY003)
665-665: Avoid specifying long messages outside the exception class
(TRY003)
697-697: Avoid specifying long messages outside the exception class
(TRY003)
702-702: Avoid specifying long messages outside the exception class
(TRY003)
706-707: Avoid specifying long messages outside the exception class
(TRY003)
706-706: f-string without any placeholders
Remove extraneous f prefix
(F541)
707-707: f-string without any placeholders
Remove extraneous f prefix
(F541)
720-720: Avoid specifying long messages outside the exception class
(TRY003)
725-725: Avoid specifying long messages outside the exception class
(TRY003)
729-730: Avoid specifying long messages outside the exception class
(TRY003)
729-729: f-string without any placeholders
Remove extraneous f prefix
(F541)
730-730: f-string without any placeholders
Remove extraneous f prefix
(F541)
745-745: Avoid specifying long messages outside the exception class
(TRY003)
pysyncrosim/helper.py
298-298: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (13)
pysyncrosim/helper.py (4)
239-239: LGTM!The key name change from
"Id"to"ProjectId"correctly extracts the project ID from the DataFrame after the rename that occurs in__init_projects().
271-271: LGTM!The key name change from
"Id"to"ScenarioId"correctly aligns with the DataFrame column after the rename in__init_scenarios().
280-303: LGTM!The
_delete_folderfunction correctly validates folder existence and executes the delete command. The implementation follows the established patterns of other delete helpers in this file.
305-331: LGTM!The
_delete_datafunction properly handles datasheet deletion with optional scoping by project/scenario ID and row filtering by IDs. The implementation is consistent with the established patterns in this module.pysyncrosim/project.py (1)
327-331: LGTM!The scenario and project deletion branches correctly delegate to
library.deletewith the appropriate parameters, and theforceparameter is properly passed through.tests/test_pysyncrosim.py (3)
352-358: LGTM!The error message expectations have been updated to lowercase format (
"project 2 does not exist"and"scenario 50 does not exist"), which matches the actual error messages inlibrary.py.
363-379: LGTM!The folder deletion tests properly cover:
- Type validation (TypeError for invalid folder type)
- Existence validation (ValueError for non-existent folder ID)
- Deletion by Folder object and by integer ID
- Verification that deleted folders are removed from the library
388-395: LGTM!The library deletion tests properly verify that the library file is removed after deletion and that attempting to delete a non-existent library raises an appropriate error.
pysyncrosim/library.py (5)
661-665: LGTM!The type checks for
folderanddatasheetparameters are correct. Thefoldercheck properly handles bothps.Folderinstances and integers (includingnp.int64).
681-687: LGTM!The library deletion branch correctly triggers when no specific deletion target (project, scenario, folder, datasheet) is specified, and properly passes through all backup/publish removal options.
689-710: LGTM!The project deletion branch properly handles all input types (int, np.int64, str, Project instance), validates existence, and delegates to the helper function.
712-734: LGTM!The scenario deletion branch properly handles all input types, validates existence, and delegates to the helper function with appropriate parameters.
736-748: LGTM with minor note.The folder deletion branch correctly handles folder IDs and Folder instances. The else branch at lines 744-745 is technically unreachable due to the earlier type validation at lines 661-663, but it serves as defensive programming.
✏️ 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: 4
🤖 Fix all issues with AI agents
In `@pysyncrosim/project.py`:
- Around line 323-324: The call to self.library.delete(...) inside the
Project.delete method ignores the ids parameter declared on the method
signature, preventing row-level deletions; update the call to include ids=ids so
it invokes self.library.delete(datasheet=datasheet, pid=self.pid, ids=ids,
force=force) (preserving existing arguments) so specific row IDs are forwarded
to the library delete implementation.
In `@pysyncrosim/scenario.py`:
- Around line 537-538: The datasheet deletion call in Scenario currently calls
self.library.delete(datasheet=datasheet, ids=ids, force=force) without scoping
to this Scenario; update that call to include sid=self.sid (matching how
Project.delete passes pid=self.pid) so the deletion is properly limited to this
Scenario (look for the delete invocation in scenario.py where datasheet is not
None).
In `@tests/test_pysyncrosim.py`:
- Line 447: The test calls myProject.save_datasheet with the wrong keyword arg
name; change the argument from datasheet= to data= so it matches the
save_datasheet signature (i.e., update the call to
myProject.save_datasheet(name="stsim_Stratum", data=test_data)). Ensure any
other tests or usages of save_datasheet use data= as well.
- Line 428: The test calls myLibrary.delete(data=True,
datasheet="stsim_Stratum", pid=myProject.pid, force=True) but Library.delete has
no data parameter; remove the unsupported data=True argument and call
myLibrary.delete(datasheet="stsim_Stratum", pid=myProject.pid, force=True) (or
pass the correct existing parameter if intended) so the call matches the
Library.delete signature.
♻️ Duplicate comments (1)
tests/test_pysyncrosim.py (1)
450-450: Fix f-string syntax for Python 3.11 compatibility.The f-string uses mixed quote styles - the second dictionary accessor uses double quotes inside a double-quoted f-string, which is invalid syntax in Python < 3.12. The project targets Python 3.11 per
.readthedocs.yaml.Proposed fix
- ids_to_delete = f"{saved_data.iloc[0]['StratumId']},{saved_data.iloc[1]["StratumId"]}" + ids_to_delete = f"{saved_data.iloc[0]['StratumId']},{saved_data.iloc[1]['StratumId']}"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pysyncrosim/project.pypysyncrosim/scenario.pytests/test_pysyncrosim.py
🧰 Additional context used
🧬 Code graph analysis (3)
pysyncrosim/scenario.py (2)
pysyncrosim/project.py (2)
delete(300-330)library(76-86)pysyncrosim/library.py (1)
delete(616-748)
tests/test_pysyncrosim.py (1)
pysyncrosim/library.py (3)
delete(616-748)folders(513-523)session(92-102)
pysyncrosim/project.py (2)
pysyncrosim/scenario.py (3)
delete(516-542)library(104-114)project(91-101)pysyncrosim/library.py (1)
delete(616-748)
🪛 Ruff (0.14.11)
pysyncrosim/project.py
300-300: Unused method argument: ids
(ARG002)
🔇 Additional comments (5)
pysyncrosim/scenario.py (1)
540-542: LGTM!The Scenario deletion path correctly forwards the
forceparameter and properly identifies the Scenario to delete.pysyncrosim/project.py (1)
326-330: LGTM!The Scenario and Project deletion paths correctly forward the
forceparameter and identify the correct entities to delete.tests/test_pysyncrosim.py (3)
364-379: LGTM!The folder deletion tests properly cover error cases (invalid type, non-existent ID) and successful deletion paths. The folders are correctly created before deletion testing, addressing the previously flagged undefined variable issue.
388-395: LGTM!The library deletion tests correctly verify the library file is removed and that subsequent deletion attempts raise an appropriate error.
451-455: LGTM!The row-level deletion test logic and assertions are correct, pending the fixes noted above.
✏️ 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 `@tests/test_pysyncrosim.py`:
- Around line 450-455: The f-string on the ids_to_delete assignment mixes double
quotes inside a double-quoted f-string causing a SyntaxError; replace the inner
double quotes with single quotes (or build the two id strings separately and
join) so the expression uses consistent quoting when accessing
saved_data.iloc[0]['StratumId'] and saved_data.iloc[1]['StratumId'], then pass
the resulting comma-joined string to myLibrary.delete(datasheet="stsim_Stratum",
pid=myProject.pid, ids=ids_to_delete, force=True).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pysyncrosim/project.pypysyncrosim/scenario.pytests/test_pysyncrosim.py
🚧 Files skipped from review as they are similar to previous changes (1)
- pysyncrosim/scenario.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_pysyncrosim.py (1)
pysyncrosim/library.py (5)
delete(616-748)name(105-115)name(118-121)folders(513-523)session(92-102)
🔇 Additional comments (3)
pysyncrosim/project.py (1)
300-331: LGTM! The delete method correctly routes to library.delete with proper parameters.The implementation properly handles all three deletion targets (datasheet, scenario, project) and passes the required parameters including
pid,ids, andforce.Minor style note: Line 324-325 has inconsistent indentation on the continuation line, and line 328 has spaces around
=in keyword arguments which differs from lines 324-325. Consider aligning for consistency, but this is not blocking.tests/test_pysyncrosim.py (2)
352-396: LGTM! Comprehensive folder and library deletion tests.The tests properly validate:
- Error messages for non-existent projects/scenarios
- Folder type validation and non-existent folder handling
- Folder deletion by object and by ID with assertions
- Library deletion with existence verification
398-449: LGTM! Thorough datasheet deletion test coverage.The tests properly exercise:
- Type validation for
datasheet,pid, andsidparameters- Empty datasheet name validation
- Deletion at project scope via
Library.delete()andProject.delete()- Deletion at scenario scope via
Library.delete()andScenario.delete()
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| f"Integer, or String") | ||
|
|
||
| helper._delete_project(library=self, name=p.name, | ||
| pid=p.pid, session=self.session, |
There was a problem hiding this comment.
I think we still want to include the pid argument here because there could be cases where you have 2 projects with the same name
| helper._delete_scenario(library=self, project=s.project, | ||
| name=s.name, sid=s.sid, | ||
| session=self.session, | ||
| helper._delete_scenario(library=self, project=s.project, |
There was a problem hiding this comment.
Same here, I think we still want the sid argument in here because you could have situations where there are scenarios with the same name but different IDs
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.