Updated InterfaceProtocolCheckMixin & Added corresponding Tests as well ( tests_minxin ) #72

RISHIKESHk07 · 2024-09-11T17:01:12Z

Related Issue

Changes Done

Updated the InterfaceProtocolCheckMixin in minxin file in utils folder
Added a line of code to check if a method is properly declared in the subclass /child class.
Wrote a Tests for the above in filder tests/tests_minxin.py

Summary by Sourcery

Update the InterfaceProtocolCheckMixin to include a check for method declaration in subclasses and add corresponding tests to validate this functionality.

New Features:

Introduce a check in the InterfaceProtocolCheckMixin to ensure methods are properly declared or overridden in subclasses.

Tests:

Add tests for InterfaceProtocolCheckMixin to verify that methods are correctly implemented in subclasses, including tests for both correct and incorrect implementations.

sourcery-ai · 2024-09-11T17:01:17Z

Reviewer's Guide by Sourcery

This pull request updates the InterfaceProtocolCheckMixin class in the utils/mixins.py file to improve method declaration checks in subclasses. It also adds corresponding tests in a new file tests/tests_mixin.py to verify the functionality of the mixin.

File-Level Changes

Change	Details	Files
Enhanced method declaration check in InterfaceProtocolCheckMixin	Added try-except block to handle potential AttributeError Implemented check to ensure method is declared/overridden in subclass Added NotImplementedError raise for inherited but not overridden methods Added NotImplementedError raise for methods not found in parent class	`xcov19/utils/mixins.py`
Added unit tests for InterfaceProtocolCheckMixin	Created test for incorrect implementation (missing method) Created test for correct implementation Implemented BaseClass for testing purposes Added assertions to verify expected behavior	`xcov19/tests/tests_mixin.py`

Tips

Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
Continue your discussion with Sourcery by replying directly to review comments.
You can change your review settings at any time by accessing your dashboard:
- Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
- Change the review language;
You can always contact us if you have any questions or feedback.

sourcery-ai

We've reviewed this pull request using the Sourcery rules engine. If you would also like our AI-powered code review then let us know.

codecakes

Need refactoring and improvements.

codecakes · 2024-09-11T21:49:07Z

xcov19/utils/mixins.py

+                if defined_method in cls.__dict__:
+                    pass
+                else:
+                    raise NotImplementedError(
+                        f"The method '{defined_method}' is inherited from the parent class '{parent_class.__name__}' and not overridden/declared."
+                    )


this is a redundant overkill. L49 does exactly this, even if you remove this line.

would replacing this with commented L49 , would be fine ?

Is there a case where this is possible?

f"The method '{defined_method}' is inherited from the parent class '{parent_class.name}' and not overridden/declared."

when this part of the code kicks in?:
https://github.com/Xcov19/project-healthcare/blob/c5cdf54cf07d32f7d259d26f0a260b292da1537c/xcov19/utils/mixins.py#L49

Your block is redundant because you're reimplementing the same checks.

codecakes · 2024-09-11T21:55:22Z

xcov19/utils/mixins.py

+                cls_method = getattr(parent_class, defined_method)
+                subclass_method = getattr(cls, defined_method)


Is this even needed scoped inside?

codecakes · 2024-09-11T22:00:09Z

xcov19/utils/mixins.py

+                    )
+            except AttributeError:
+                raise NotImplementedError(
+                    f"Method '{defined_method}' not found in parent class."


what if defined_method is defined in cls but not in parent_class? Could the try/except block be refactored to raise this part and keep the rest of the blocks same?

cls_method = getattr(parent_class, defined_method)
i thought this line would take care of that , thats why i kept it in the try catch block , correct me if i am wrong

There are 2 things you can do to make this clearer:

Suggestion 1

Change:
cls_method = getattr(parent_class, defined_method)
to:
parent_cls_method = getattr(parent_class, defined_method)

because parent_class -> parent_cls_method, looks more logical and then there won't be any confusion between parent_cls_method and subclass_method. You would need to make the change from cls_method-> parent_cls_method everywhere else in the code. A good IDE will help here.

Suggestion 2

You're removing this into a try block to capture exception:
https://github.com/Xcov19/project-healthcare/blob/c5cdf54cf07d32f7d259d26f0a260b292da1537c/xcov19/utils/mixins.py#L56

here:

except AttributeError:

Another way to refactor the class could be:

if not ( hasattr(parent_class, defined_method) and (parent_cls_method := getattr(parent_class, defined_method)) ): # you raise here

This way you wouldn't need to do alot of bloated checks in the block you have introduced and you'd trim down the lines.

@codecakes sorry about the hiatus , over here when we use hasattr the subclass method which is inherited from parent would make this True , so here we cannot differentiate whether this was inherited or overidden , it was this case which made me forget about completing the issue .

codecakes · 2024-09-11T22:01:23Z

xcov19/tests/tests_mixin.py

+    # Define IncorrectImplementation inside the test function
+    try:
+
+        class IncorrectImplementation(BaseClass, InterfaceProtocolCheckMixin):


I am of the opinion that stub classes should be defined already. Included in the file before these test cases.

codecakes · 2024-09-11T22:01:35Z

xcov19/tests/tests_mixin.py

+
+
+def test_correct_implementation():
+    class CorrectImplementation(BaseClass, InterfaceProtocolCheckMixin):


ditto as above

codecakes

i hope this helps.

codecakes · 2024-09-12T08:15:01Z

xcov19/utils/mixins.py

+                if defined_method in cls.__dict__:
+                    pass
+                else:
+                    raise NotImplementedError(
+                        f"The method '{defined_method}' is inherited from the parent class '{parent_class.__name__}' and not overridden/declared."
+                    )


Is there a case where this is possible?

f"The method '{defined_method}' is inherited from the parent class '{parent_class.name}' and not overridden/declared."

when this part of the code kicks in?:
https://github.com/Xcov19/project-healthcare/blob/c5cdf54cf07d32f7d259d26f0a260b292da1537c/xcov19/utils/mixins.py#L49

Your block is redundant because you're reimplementing the same checks.

codecakes · 2024-09-12T08:27:02Z

xcov19/utils/mixins.py

+                    )
+            except AttributeError:
+                raise NotImplementedError(
+                    f"Method '{defined_method}' not found in parent class."


There are 2 things you can do to make this clearer:

Suggestion 1

Change:
cls_method = getattr(parent_class, defined_method)
to:
parent_cls_method = getattr(parent_class, defined_method)

because parent_class -> parent_cls_method, looks more logical and then there won't be any confusion between parent_cls_method and subclass_method. You would need to make the change from cls_method-> parent_cls_method everywhere else in the code. A good IDE will help here.

Suggestion 2

You're removing this into a try block to capture exception:
https://github.com/Xcov19/project-healthcare/blob/c5cdf54cf07d32f7d259d26f0a260b292da1537c/xcov19/utils/mixins.py#L56

here:

except AttributeError:

Another way to refactor the class could be:

if not ( hasattr(parent_class, defined_method) and (parent_cls_method := getattr(parent_class, defined_method)) ): # you raise here

This way you wouldn't need to do alot of bloated checks in the block you have introduced and you'd trim down the lines.

codecakes · 2024-09-12T08:30:18Z

xcov19/tests/tests_mixin.py

+
+

You could simply move the setup before actual tests so the definitions would be clearer to read and your code would only instantiate it.

Suggested change

class IncorrectImplementation(BaseClass, InterfaceProtocolCheckMixin):

"""Does not implement method_with_params"""

pass

codecakes · 2024-09-12T08:42:43Z

@sourcery-ai review

sourcery-ai

We've reviewed this pull request using the Sourcery rules engine. If you would also like our AI-powered code review then let us know.

codecakes · 2024-09-13T08:33:51Z

@sourcery-ai review

sourcery-ai

Hey @RISHIKESHk07 - I've reviewed your changes - here's some feedback:

Overall Comments:

Consider simplifying the method checking logic in __init_subclass__ to avoid nested try-except blocks and reduce the use of exceptions for control flow.
The TODO comment in __init_subclass__ has been addressed and can be removed.

Here's what I looked at during the review

🟡 General issues: 1 issue found
🟢 Security: all looks good
🟡 Review instructions: 1 issue found
🟢 Testing: all looks good
🟢 Complexity: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.}

sourcery-ai · 2024-09-13T08:35:07Z

xcov19/utils/mixins.py

+                if defined_method in cls.__dict__:
+                    pass


suggestion (review_instructions): Consider removing unnecessary 'pass' statement.

The 'pass' statement here is not needed as the 'if' block can be empty. This would make the code more concise.

if defined_method not in cls.__dict__: raise NotImplementedError(

Review instructions:

Path patterns: **/*.py, **/*.js, **/*.ex

Instructions:

Check code for Google style guide for python code.

Review code for Top 10 OWASP recommendations.

sourcery-ai · 2024-09-13T08:35:08Z

xcov19/utils/mixins.py

@@ -53,8 +52,24 @@ def __init_subclass__(cls, **kwargs):
            if not method_name.startswith("__")
        ):
            # TODO: Raise if either classes don't have the method declared.


suggestion: Update or remove the TODO comment

This TODO comment seems to be addressed by the new code. Consider removing it or updating it to reflect any remaining tasks.

# Ensure both classes have the method declared before proceeding

sonarcloud · 2024-09-14T11:37:47Z

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

codecakes

@RISHIKESHk07 added some more feedback for scope for code improvement.

codecakes · 2024-09-14T12:01:24Z

xcov19/tests/tests_mixin.py

+class BaseClass:
+    @classmethod
+    def method_with_params(cls, param1: int, param2: str) -> None:
+        pass


Add a nested comment explaining what this method is achieves even though it is empty.

codecakes · 2024-09-14T12:02:09Z

xcov19/tests/tests_mixin.py

+    class CorrectImplementation(BaseClass, InterfaceProtocolCheckMixin):
+        @classmethod
+        def method_with_params(cls, param1: int, param2: str) -> None:
+            pass


same here. Add a nested comment.

codecakes · 2024-09-14T12:04:36Z

xcov19/tests/tests_mixin.py

+    try:
+
+        class IncorrectImplementation(BaseClass, InterfaceProtocolCheckMixin):
+            """Does not implement method_with_params"""
+
+            pass
+
+        IncorrectImplementation()
+
+    except NotImplementedError as exec:


replace with self.assertRaises or if not using unittest.TestCaset then pytest.raises; same in everywhere else.

codecakes · 2024-09-14T12:05:14Z

xcov19/tests/tests_mixin.py

+    try:
+        instance = CorrectImplementation()
+        instance.method_with_params(1, "test")
+        assert True


assert raise or assert on instance. assert True is an oxymoron.

VedantKhairnar · 2024-09-16T09:01:14Z

Hello @RISHIKESHk07
We have extended the deadline of PR merge of Augtoberfest to tomorrow EoD. If you can implement the required changes soon, we will be able to merge it by tomorrow.
Thanks.

RISHIKESHk07 · 2024-09-16T09:19:36Z

Thanks for that @VedantKhairnar , due to academic exams I have been inactive for a few days will try work on the issue as time permits

RISHIKESHk07 · 2024-11-18T21:16:01Z

@codecakes Sorry about the hiatus , did add a extra doubt above in the minxins.py conversations

sourcery-ai bot reviewed Sep 11, 2024

View reviewed changes

Updated minxins File & Added corresponding Tests as well tests_minxin

c5cdf54

RISHIKESHk07 force-pushed the test_minxin branch from dd6163a to c5cdf54 Compare September 11, 2024 17:02

codecakes self-requested a review September 11, 2024 21:30

codecakes assigned RISHIKESHk07 Sep 11, 2024

codecakes requested changes Sep 11, 2024

View reviewed changes

codecakes requested changes Sep 12, 2024

View reviewed changes

sourcery-ai bot reviewed Sep 12, 2024

View reviewed changes

sourcery-ai bot approved these changes Sep 13, 2024

View reviewed changes

codecakes added bug Something isn't working Mend: configuration error Mend configuration error labels Sep 14, 2024

codecakes linked an issue Sep 14, 2024 that may be closed by this pull request

[C4GT Community]: Raise NotImplementedError in InterfaceProtocolCheckMixin if either classes lack the declared method. #33

Open

4 tasks

codecakes added this to the v1 milestone Sep 14, 2024

codecakes requested changes Sep 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated InterfaceProtocolCheckMixin & Added corresponding Tests as well ( tests_minxin ) #72

Updated InterfaceProtocolCheckMixin & Added corresponding Tests as well ( tests_minxin ) #72

RISHIKESHk07 commented Sep 11, 2024 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Sep 11, 2024 •

edited

Loading

sourcery-ai bot left a comment

codecakes left a comment

codecakes Sep 11, 2024

RISHIKESHk07 Sep 12, 2024

codecakes Sep 12, 2024

codecakes Sep 11, 2024

codecakes Sep 11, 2024

RISHIKESHk07 Sep 12, 2024

codecakes Sep 12, 2024

RISHIKESHk07 Nov 18, 2024

codecakes Sep 11, 2024

codecakes Sep 11, 2024

codecakes left a comment

codecakes Sep 12, 2024

codecakes Sep 12, 2024

codecakes Sep 12, 2024

codecakes commented Sep 12, 2024

sourcery-ai bot left a comment

codecakes commented Sep 13, 2024

sourcery-ai bot left a comment

sourcery-ai bot Sep 13, 2024

sourcery-ai bot Sep 13, 2024

sonarcloud bot commented Sep 14, 2024

codecakes left a comment

codecakes Sep 14, 2024

codecakes Sep 14, 2024

codecakes Sep 14, 2024

codecakes Sep 14, 2024

VedantKhairnar commented Sep 16, 2024

RISHIKESHk07 commented Sep 16, 2024

RISHIKESHk07 commented Nov 18, 2024

		cls_method = getattr(parent_class, defined_method)
		subclass_method = getattr(cls, defined_method)



		def test_correct_implementation():
		class CorrectImplementation(BaseClass, InterfaceProtocolCheckMixin):

Updated InterfaceProtocolCheckMixin & Added corresponding Tests as well ( tests_minxin ) #72

Are you sure you want to change the base?

Updated InterfaceProtocolCheckMixin & Added corresponding Tests as well ( tests_minxin ) #72

Conversation

RISHIKESHk07 commented Sep 11, 2024 • edited by sourcery-ai bot Loading

Related Issue

Changes Done

Summary by Sourcery

sourcery-ai bot commented Sep 11, 2024 • edited Loading

Reviewer's Guide by Sourcery

File-Level Changes

sourcery-ai bot left a comment

Choose a reason for hiding this comment

codecakes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Suggestion 1

Suggestion 2

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecakes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Suggestion 1

Suggestion 2

Choose a reason for hiding this comment

codecakes commented Sep 12, 2024

sourcery-ai bot left a comment

Choose a reason for hiding this comment

codecakes commented Sep 13, 2024

sourcery-ai bot left a comment

Choose a reason for hiding this comment

sourcery-ai bot Sep 13, 2024

Choose a reason for hiding this comment

sourcery-ai bot Sep 13, 2024

Choose a reason for hiding this comment

sonarcloud bot commented Sep 14, 2024

Quality Gate passed

codecakes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VedantKhairnar commented Sep 16, 2024

RISHIKESHk07 commented Sep 16, 2024

RISHIKESHk07 commented Nov 18, 2024

RISHIKESHk07 commented Sep 11, 2024 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Sep 11, 2024 •

edited

Loading