Use location= for parent/child connection
#125
Conversation
WalkthroughThis pull request introduces significant refactoring and enhancements to the Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (17)
- magicclass/init.py (1 hunks)
- magicclass/_gui/_base.py (19 hunks)
- magicclass/_gui/class_gui.py (2 hunks)
- magicclass/_gui/menu_gui.py (1 hunks)
- magicclass/_gui/toolbar.py (1 hunks)
- magicclass/_gui/utils.py (1 hunks)
- magicclass/core.py (1 hunks)
- magicclass/ext/pyqtgraph/graph_items.py (1 hunks)
- magicclass/fields/_define.py (1 hunks)
- magicclass/fields/_fields.py (22 hunks)
- magicclass/utils/init.py (1 hunks)
- magicclass/utils/_functions.py (2 hunks)
- magicclass/wrappers/_abstractapi.py (5 hunks)
- magicclass/wrappers/_misc.py (7 hunks)
- tests/test_bind.py (3 hunks)
- tests/test_callbacks.py (1 hunks)
- tests/test_wraps.py (2 hunks)
Files skipped from review due to trivial changes (6)
- magicclass/init.py
- magicclass/_gui/class_gui.py
- magicclass/core.py
- magicclass/ext/pyqtgraph/graph_items.py
- magicclass/fields/_define.py
- tests/test_callbacks.py
Additional comments (Suppressed): 51
magicclass/utils/__init__.py (1)
- 3-9: The new function
get_levelhas been added to the import list. Ensure that this function is implemented correctly and used appropriately throughout the codebase.magicclass/_gui/menu_gui.py (1)
- 89-97: The new hunk has removed the handling of
FunctionGuiandMagicFieldinstances, and the binding ofselfto the first argument ofFunctionGui. If these changes are intentional, ensure that they do not break existing functionality. Also, the new hunk does not handle the case whereattris a type. If this is intentional, verify that it does not affect the expected behavior.- if isinstance(attr, type): - # Nested magic-menu - widget = attr() - object.__setattr__(self, name, widget) - - elif isinstance(attr, MagicField): - widget = self._create_widget_from_field(name, attr) - if not widget.tooltip: - widget.tooltip = _tooltips.attributes.get(name, "") - - else: - # convert class method into instance method - widget = getattr(self, name, None) - - if isinstance(widget, FunctionGui): - widget = attr.copy() - widget[0].bind(self) # bind self to the first argument - - elif isinstance(widget, BaseGui): - connect_magicclasses(self, widget, name)magicclass/_gui/toolbar.py (2)
- 130-132: The new hunk has removed the checks for
attrbeing a type,MagicField, orFunctionGui. Ensure that these cases are handled elsewhere or are no longer necessary. If they are still necessary, consider adding them back.- if isinstance(attr, type): - # Nested magic-menu - widget = attr() - object.__setattr__(self, name, widget) - - elif isinstance(attr, MagicField): - widget = self._create_widget_from_field(name, attr) - if not widget.tooltip: - widget.tooltip = _tooltips.attributes.get(name, "") - - else: - # convert class method into instance method - widget = getattr(self, name, None) - - if isinstance(widget, FunctionGui): - widget = attr.copy() - widget[0].bind(self) # bind self to the first argument + widget = self._convert_an_attribute_into_widget(name, attr, _tooltips)
- 133-135: The new hunk has removed the
connect_magicclasses(self, widget, name)call. Ensure that this connection is established elsewhere or is no longer necessary. If it is still necessary, consider adding it back.- connect_magicclasses(self, widget, name) + # No equivalent line in new hunktests/test_wraps.py (2)
- 175-182: The
fieldfunction now accepts alocationparameter, which is a change from the previous version where thefieldfunction was called on an object. Ensure that all calls to this function throughout the codebase have been updated to match the new usage.- a = B.field(int) - b = B.field(str) + a = field(int, location=B) + b = field(str, location=B)
- 203-210: Similar to the
fieldfunction, thevfieldfunction now also accepts alocationparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new usage.- a = B.vfield(int) - b = B.vfield(str) + a = vfield(int, location=B) + b = vfield(str, location=B)magicclass/_gui/utils.py (2)
- 46-65: The
copy_classfunction has been updated to handle more cases. The new code checks if the attribute is an instance ofFunctionType,abstractapi, orthread_worker. If it is, it updates the__qualname__attribute. If the attribute is a type, it makes a recursive call tocopy_class. The function also now skips copying the__original_class__attribute and sets it on the copied class at the end. Ensure that these changes do not introduce any unexpected behavior or side effects.- if isinstance(attr, FunctionType): + if isinstance(attr, (FunctionType, abstractapi, thread_worker)):- attr = copy_class(attr, qualname, attr.__name__) + attr = copy_class(attr, qualname, attr.__qualname__.split(".")[-1])+ out.__original_class__ = getattr(cls, "__original_class__", cls)
- 52-53: The
__original_class__attribute is skipped during the copying process. This is a new behavior and should be verified to ensure it doesn't cause any issues.+ if key == ("__original_class__"): + continuemagicclass/utils/_functions.py (3)
129-133: The
get_levelfunction is introduced to determine the level of a callable or type based on its qualified name. It subtracts one from the level if the input is callable. This function seems to be used for determining the nesting level of a function or class within a module or class. Ensure that this function is used correctly throughout the codebase.136-136: The
_LOCALSvariable is moved from its previous location. This change doesn't affect the functionality, but it's important to ensure that this variable is defined before it's used in the code.146-161: The logic for determining the namespace of a method and checking its visibility from a class has been slightly modified. The
self_clsvariable is now determined before the visibility check, and theinsvariable is initialized earlier. These changes don't seem to affect the overall functionality, but it's important to verify that they don't introduce any subtle bugs.tests/test_bind.py (4)
1-6: The import of
set_design,get_button,magicmenu, andabstractapihas been added, andset_optionshas been removed. Ensure that these changes are reflected in the rest of the codebase.185-191: The
Boundtype has been replaced withAnnotatedfor binding. This change should be verified across the codebase to ensure consistency.- def func(self, x: Bound[b.x]): + def func(self, x: Annotated[int, {"bind": b.x}]):
198-234: Two new test cases
test_wrapped_with_external_fieldandtest_wrapped_with_external_field_via_fieldhave been added. These tests seem to be checking the functionality of theset_designfunction with thelocationparameter and theAnnotatedtype for binding. Ensure that these tests cover all edge cases and possible scenarios.270-270: The
test_multi_guifunction has been moved down in the file. Ensure that this does not affect the execution order of the tests or their dependencies.magicclass/wrappers/_abstractapi.py (5)
1-13: The import of
inspectmodule is new in this hunk. It is used later in the code for creating an empty signature for theabstractapiwhen a location is provided. The rest of the imports and the class definition remain unchanged.34-42: The
__init__method ofabstractapinow accepts an additional keyword argumentlocation. This argument is expected to be of typeMagicTemplateorMagicFieldorNone. Thefuncargument validation remains the same.46-52: The
locationargument is stored in the_locationattribute of theabstractapiinstance. If alocationis provided and theabstractapiinstance does not have a__signature__attribute, an emptyinspect.Signatureis assigned to__signature__. This change might affect the behavior of theabstractapiinstance when it is used as a decorator for functions or methods without a signature.58-66: In the
__set_name__method, theabstractapiinstance now checks if there is anabstractapiinstance in its_locationwith the same name. If there is, it resolves thatabstractapiinstance and assigns an emptyinspect.Signatureto it if it does not have a__signature__. This change might affect the behavior of theabstractapiinstance when it is used as a decorator for functions or methods without a signature.84-97: A new method
get_locationis added to theabstractapiclass. This method returns the_locationattribute of theabstractapiinstance. If the_locationis an instance ofMagicField, it returns theconstructorattribute of theMagicFieldinstance. This method provides a way to retrieve the location of theabstractapiinstance.magicclass/_gui/_base.py (17)
44-62: Imports have been modified. Ensure that the removed import
PushButtonPlusis not used elsewhere in the codebase. Also, verify that the newly added importsFunctionGuiPlus,Clickable,is_clickableare used correctly in the codebase.72-78: The import
MagicValueFieldhas been removed. Ensure that it is not used elsewhere in the codebase.192-214: The type of
_component_classhas been changed fromtype[Action | PushButtonPlus]totype[Clickable]and the type of_listhas been changed fromlist[Action | Widget]tolist[AbstractAction | Widget]. Ensure that these changes do not break the existing functionality.362-365: The
typefunction has been replaced with_typeoffunction. Ensure that_typeoffunction is defined and works as expected.440-452: The logic for handling
predefinedhas been modified. Ensure that the new logic works as expected and does not introduce any bugs.474-483: The variable
clshas been replaced withinto_cls. Ensure thatinto_clsis defined and works as expected.515-528: The variable
child_instancehas been replaced withchild_ins. Ensure thatchild_insis defined and works as expected.532-558: The variable
child_instancehas been replaced withchild_ins. Ensure thatchild_insis defined and works as expected.566-573: The error message in the
RuntimeErrorhas been modified. Ensure that the new error message is accurate and informative.580-600: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [580-623]
The
fieldandvfieldmethods have been modified to include a deprecation warning. Ensure that the new methods work as expected and the deprecation warning is accurate.
746-753: The
lenfunction has been replaced with asumfunction. Ensure that the new logic works as expected and does not introduce any bugs.830-835: The type of
widget_has been changed fromAction | PushButtonPlustoClickable. Ensure that this change does not break the existing functionality.843-857: The
_iter_child_magicclassesmethod has been modified to includelevelandmax_levelparameters. Ensure that the new logic works as expected and does not introduce any bugs.1149-1152: The type of
widget_has been changed fromAction | PushButtonPlustoClickable. Ensure that this change does not break the existing functionality.1229-1239: The variable
child_clsnamehas been replaced withinto. Ensure thatintois defined and works as expected.1308-1320: The logic for handling
objhas been modified. Ensure that the new logic works as expected and does not introduce any bugs.1342-1345: The type of
widgethas been changed fromPushButtonPlus | ActiontoClickable. Ensure that this change does not break the existing functionality.magicclass/fields/_fields.py (7)
32-38: The import of
get_levelis new. Ensure that it is used appropriately in the code and that it does not conflict with any existing names.216-219: The method
with_widget_typeno longer creates a new copy of the object before setting the_widget_type. This changes the behavior of the method and may have unintended side effects if the original object was intended to be immutable. Verify that this change is intentional and does not introduce bugs.365-379: The logic for determining the namespace of
MagicFieldhas been changed. Previously, it was based on the class name, but now it's based on the qualified name. This could potentially change the behavior of the_funcmethod. Ensure that this change is intentional and that it does not introduce bugs.609-612: The return value of the
_wrapperfunction has been changed from_afunc.replace(force_async=False)toself.connect(_afunc). This could potentially change the behavior of the function. Ensure that this change is intentional and that it does not introduce bugs.1037-1041: The
locationparameter has been added to several function signatures. Ensure that all calls to these functions throughout the codebase have been updated to match the new signatures.934-947: The
locationparameter is now used to set the destination of theMagicFieldobject. Ensure that this change is intentional and that it does not introduce bugs.1086-1092: The
locationparameter is now used to set the destination of theMagicValueFieldobject. Ensure that this change is intentional and that it does not introduce bugs.magicclass/wrappers/_misc.py (7)
9-14: The import statements have been updated to reflect the changes in the codebase. The
BaseGuiclass has been replaced withMagicTemplateandMagicFieldclasses. Ensure that these classes provide the same or improved functionality as theBaseGuiclass.61-74: The
set_optionsfunction now checks if theoptionsparameter is not empty before proceeding with the function logic. This is a good practice as it prevents unnecessary operations whenoptionsis empty.95-101: The
set_designfunction now accepts alocationparameter of typeMagicTemplateorMagicField. This allows the function to set the location of the widget in the GUI. Ensure that this change is reflected in all calls toset_design.130-136: The
set_designfunction now removes thelocationparameter fromcaller_optionsbefore proceeding with the function logic. This is necessary becauselocationis not a valid option for theupgrade_signaturefunction.145-152: The
set_designfunction now checks iflocationis notNoneand if so, calls thewrapsmethod onlocationwithobjas the argument. This sets the location of the widget in the GUI. Ensure that thewrapsmethod is implemented correctly in theMagicTemplateandMagicFieldclasses.246-252: The
setup_function_guifunction now accepts asetupparameter of typeCallable[[MagicTemplate, FunctionGui], None]instead ofCallable[[BaseGui, FunctionGui], None]. This change reflects the replacement of theBaseGuiclass with theMagicTemplateclass. Ensure that this change is reflected in all calls tosetup_function_gui.255-261: The
_setupfunction inside thesetup_function_guifunction now accepts aselfparameter of typeMagicTemplateinstead ofBaseGui. This change reflects the replacement of theBaseGuiclass with theMagicTemplateclass. Ensure that this change is reflected in all calls to_setup.
| @wraps(fn) | ||
| def _func(self: MagicTemplate, *args, **kwargs): | ||
| nonlocal _running, _last_run, _last_run_id, _abort_begin | ||
|
|
||
| with threading.Lock(): | ||
| _this_id = default_timer() | ||
| f = _afunc.__get__(self) |
There was a problem hiding this comment.
The use of threading.Lock() without a variable assignment means that a new lock is created and immediately discarded each time the with block is entered. This will not provide any synchronization because each thread will have its own lock. If the intention was to synchronize access to _running, _last_run, _last_run_id, and _abort_begin, a shared lock should be used.
- with threading.Lock():
+ with shared_lock:
Closes #124
<class name>.wrapsand<class name>.fieldare kind of inconsistent.Moving the location of widgets are now refactored as
location=keyword argument.@set_design(location=X)instead of@X.wraps.field(..., location=X)instead ofX.field(...).Summary by CodeRabbit
New Features:
locationparameter tofieldandvfieldfunctions for specifying the location of fields.get_levelfunction to determine the level of a function in the class hierarchy.Bug Fixes:
method_as_getterfunction where it was not correctly identifying the class name for nested classes.Refactor:
_convert_attributes_into_widgetsmethod inmenu_gui.pyandtoolbar.pyfor improved modularity and readability.abstractapiclass to handle a new optionallocationargument.Test:
test_wrapped_with_external_fieldandtest_wrapped_with_external_field_via_fieldintest_bind.py.test_callbacks.pyandtest_wraps.pyto reflect changes infieldandvfieldfunctions.