Pylint fixes #59

adarshk7 · 2021-08-19T13:49:47Z

Closes #47

src/flexmock/api.py

codecov-commenter · 2021-08-19T15:22:59Z

Codecov Report

Merging #59 (60d64fc) into master (513265e) will decrease coverage by 0.13%.
The diff coverage is 87.71%.

❗ Current head 60d64fc differs from pull request most recent head bdd6e47. Consider uploading reports for the commit bdd6e47 to get more accurate results

@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
- Coverage   97.02%   96.88%   -0.14%     
==========================================
  Files           4        4              
  Lines         772      739      -33     
==========================================
- Hits          749      716      -33     
  Misses         23       23

Impacted Files	Coverage Δ
src/flexmock/integrations.py	`80.00% <0.00%> (ø)`
src/flexmock/api.py	`98.77% <90.90%> (-0.06%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 513265e...bdd6e47. Read the comment docs.

christophe-riolo

Nice improvements in the API 👍

christophe-riolo · 2021-08-19T18:49:18Z

src/flexmock/api.py

                try:
+                    # pylint: disable=fixme
                    # TODO(herman): this is awful, fix this properly.
                    # When a class/static method is mocked out on an *instance*
                    # we need to fetch the type from the class
                    method_type = type(_getattr(obj.__class__, name))
-                except Exception:
+                except Exception:  # pylint: disable=broad-except


We could now use https://docs.python.org/3.7/library/contextlib.html#contextlib.suppress I don't think it's affected by this warning

src/flexmock/api.py

christophe-riolo · 2021-08-19T19:02:01Z

src/flexmock/api.py

+            return len(received) == len(expected) and all(
+                _arguments_match(r, e) for r, e in zip(received, expected)
+            )


We can be smart with itertools also:

Suggested change

return len(received) == len(expected) and all(

_arguments_match(r, e) for r, e in zip(received, expected)

)

return all(

_arguments_match(r, e) for r, e in itertools.zip_longest(received, expected, fillvalue=object())

)

(changing the fillvalue to a new object ensures that nothing will be equal to it)

I'm not saying we need to change it, it might also be considered too smart and harder to maintain, but I find it interesting 😄

christophe-riolo · 2021-08-19T19:06:22Z

src/flexmock/api.py

+                return_values = (
+                    (
+                        expectation._original_function(*kargs, **kwargs)
+                        if type(expectation._original) in SPECIAL_METHODS
+                        else expectation._original(runtime_self, *kargs, **kwargs)
+                    )
+                    if inspect.isclass(expectation._mock)
+                    else expectation._original(*kargs, **kwargs)
+                )


Suggested change

return_values = (

(

expectation._original_function(*kargs, **kwargs)

if type(expectation._original) in SPECIAL_METHODS

else expectation._original(runtime_self, *kargs, **kwargs)

)

if inspect.isclass(expectation._mock)

else expectation._original(*kargs, **kwargs)

)

return_values = expectation._original(*kargs, **kwargs)

if not inspect.isclass(expectation._mock)

else expectation._original_function(*kargs, **kwargs)

if type(expectation._original) in SPECIAL_METHODS

else expectation._original(runtime_self, *kargs, **kwargs)

christophe-riolo · 2021-08-19T19:08:04Z

src/flexmock/api.py

                return _handle_exception_matching(expectation)
-            expected_values = _getattr(expectation, "_return_values")
+            expected_values = expectation._return_values


I wonder what was the need for _getattr, I hope we are not breaking anything there … Have you looked up the history ?

It seems that this is an optimization to avoid calling the custom __getattribute__ that has multiple if statements:
38db75e

Maybe we could just have some hotpath for private attributes. For example:

def __getattribute__(self, name: str) -> Any: if name.startswith("_"): # Return immediately for private attributes return _getattr(self, name) if name == "once": return _getattr(self, "times")(1) if name == "twice": return _getattr(self, "times")(2) if name == "never": return _getattr(self, "times")(0) if name in ("at_least", "at_most", "ordered", "one_by_one"): return _getattr(self, name)() if name == "mock": return _getattr(self, "mock")() return _getattr(self, name)

Note that we wouldn't need the _getattr function since it's basically just an alias of object.__getattribute__

Yeah, we can get rid of the function and call object.__getattribute__. However, if we change Expectation.__getattribute__ like I suggested, then there is no need to call object.__getattribute__ (expect inside Expectation.__getattribute__ itself).

We could even replace the if-branches with a dict lookup, it would be optimized that way too, or just have those as actual attributes/methods. (I know that it was a choice made before, but I think it takes away from the maintainability of this class anyways.)

It seems that they were properties before:
365c96a

I think this change was made so that they could be called with or without parenthesis. I think it would be better to have those as actual methods because now code completion doesn't work for them.

src/flexmock/api.py

christophe-riolo · 2021-08-19T19:30:05Z

src/flexmock/api.py

        for attr in UPDATED_ATTRS:
            try:
                obj_dict = obj.__dict__
                if obj_dict[attr].__code__ is Mock.__dict__[attr].__code__:
                    del obj_dict[attr]
-            except Exception:
+            except Exception:  # pylint: disable=broad-except
                try:


You can again use contextlib.suppress here, it would look cleaner

christophe-riolo · 2021-08-19T19:33:00Z

src/flexmock/api.py

-    for obj in instances + classes:
+    saved_flexmock_objects = FlexmockContainer.flexmock_objects
+    all_expectations = list(itertools.chain.from_iterable(saved_flexmock_objects.values()))
+    for expectation in all_expectations[:]:


Why do you need to do a copy ? You are not modifying the list.

I think it's needed because Exceptation.reset() calls del self.

christophe-riolo · 2021-08-19T19:35:02Z

src/flexmock/api.py

+
+    def _is_instance_or_class(obj: Any) -> bool:
+        return bool(
+            (not isinstance(obj, Mock) and not inspect.isclass(obj)) or inspect.isclass(obj)


Is that not already a boolean ? 🤔

It is but I guess it was done this way so that pylint doesn't complain about not using the ternary operator.

christophe-riolo · 2021-08-19T19:58:15Z

tests/flexmock_test.py

 import random
 import re
 import sys
 import unittest

+import pytest


Don't use pytest here, I believe the point of this test is to use regular unittest. And seriously it's not so bad style-wise either, they are maintainable.

I was thinking about that too. I wonder if we should run tests with other test frameworks so that we only install that one test framework and not others.

Makes sense, I'll revert the change here.

ollipa

Nice improvements! 👍 My comments are mostly suggestions to improve the readability of the code.

src/flexmock/api.py

ollipa · 2021-08-20T00:44:51Z

src/flexmock/api.py

                return _handle_exception_matching(expectation)
-            expected_values = _getattr(expectation, "_return_values")
+            expected_values = expectation._return_values


It seems that this is an optimization to avoid calling the custom __getattribute__ that has multiple if statements:
38db75e

Maybe we could just have some hotpath for private attributes. For example:

def __getattribute__(self, name: str) -> Any: if name.startswith("_"): # Return immediately for private attributes return _getattr(self, name) if name == "once": return _getattr(self, "times")(1) if name == "twice": return _getattr(self, "times")(2) if name == "never": return _getattr(self, "times")(0) if name in ("at_least", "at_most", "ordered", "one_by_one"): return _getattr(self, name)() if name == "mock": return _getattr(self, "mock")() return _getattr(self, name)

src/flexmock/api.py

ollipa · 2021-08-20T00:54:35Z

src/flexmock/api.py

+            raised, instance = sys.exc_info()[:2]
+            assert raised, "no exception was raised"
+            message = "%s" % instance
+            expected = return_values[0].raises


Suggested change

expected = return_values[0].raises

expected_exception = return_values[0].raises

src/flexmock/api.py

ollipa · 2021-08-20T01:42:09Z

src/flexmock/api.py

+                return expectation._replace_with(*kargs, **kwargs)
+            return_values = expectation._return_values or [ReturnValue()]
+            return_value = return_values.pop(0)
+            return_values.append(return_value)


This is different from the existing behavior. If return_values is empty, here return_values would be [ReturnValue()] and in the existing code it is empty (ReturnValue() is never appended to return_values).

Actually you can just return here early if return_values is empty:

if not expectation._return_values: return None return_value = return_values.pop(0) return_values.append(return_value) ...

This is different from the existing behavior. If return_values is empty, here return_values would be [ReturnValue()] and in the existing code it is empty (ReturnValue() is never appended to return_values).

Actually not, because if expectation._return_value is empty, then the (mutable) reference to it is not what gets assigned to the return_values variable, so expectation._return_value would not be mutated at all in the case.

Ah, right. Very confusing 😁 Anyway I think early return when return_valuesis empty would make this better.

ollipa · 2021-08-20T01:51:52Z

src/flexmock/api.py

-                for expectation in expectations:
-                    _getattr(expectation, "reset")()
+            for expectation in itertools.chain.from_iterable(
+                FlexmockContainer.flexmock_objects.values()


Can you add a new method to FlexmockContainer that does this? For example FlexmockContainer.saved_expectation.

ollipa · 2021-08-20T01:58:21Z

src/flexmock/api.py

+    for expectation in all_expectations[:]:
+        expectation.reset()
+
+    def _is_instance_or_class(obj: Any) -> bool:


is_instance_or_class = lambda obj: (not isinstance(obj, Mock) and not inspect.isclass(obj)) or inspect.isclass(obj)

ollipa · 2021-08-20T02:04:47Z

tests/flexmock_test.py

 import random
 import re
 import sys
 import unittest

+import pytest


I was thinking about that too. I wonder if we should run tests with other test frameworks so that we only install that one test framework and not others.

christophe-riolo · 2022-01-11T11:53:30Z

@ollipa I think this PR can be closed, the code has changed so much, and the original issue is probably not valid anymore ?

ollipa · 2022-01-12T01:21:53Z

@ollipa I think this PR can be closed, the code has changed so much, and the original issue is probably not valid anymore ?

I re-enabled some globally disabled lints that didn't have any effect in #125, but otherwise I think the original issue is still valid. It would be nice to get the refactoring of _create_mock_method method merged (I intentionally didn't touch that when Adarsh was working on this PR).

@adarshk7, are you still interested on working on this? It might be better to just start a new branch instead of trying to solve all the conflicts.

ollipa · 2022-11-12T11:09:12Z

@adarshk7, I'll close this PR but feel free to pick up where you left if you have time.

adarshk7 added 12 commits August 19, 2021 16:15

Fix pylint too-many-instance-attributes

33a251f

Fix pylint too-few-public-methods

49153b4

Remove pylint disable logging-fstring-interpolation

b1cfb5a

Remove pylint disable too-many-function-args

17df468

Remove pylint disable too-many-arguments

183a411

Remove pylint disable bad-continuation

4f29c41

Remove pylint disable too-many-locals

7d6da94

Fix pylint too-many-branches

ec21974

Fix pylint too-many-public-methods

bb752bc

Fix pylint protected-access

c77e604

Fix pylint broad-except

2c52c4f

Fix pylint fixme

15a3858

adarshk7 requested review from ollipa and christophe-riolo and removed request for ollipa August 19, 2021 15:11

adarshk7 commented Aug 19, 2021

View reviewed changes

src/flexmock/api.py Outdated Show resolved Hide resolved

Fix pylint too-many-statements

bdd6e47

adarshk7 force-pushed the pylint-fixes branch from 60d64fc to bdd6e47 Compare August 19, 2021 15:30

christophe-riolo reviewed Aug 19, 2021

View reviewed changes

ollipa assigned adarshk7 Aug 20, 2021

ollipa added the refactoring label Aug 20, 2021

ollipa reviewed Aug 20, 2021

View reviewed changes

ollipa closed this Nov 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pylint fixes #59

Pylint fixes #59

adarshk7 commented Aug 19, 2021 •

edited

codecov-commenter commented Aug 19, 2021 •

edited

christophe-riolo left a comment

christophe-riolo Aug 19, 2021

christophe-riolo Aug 19, 2021

christophe-riolo Aug 19, 2021

christophe-riolo Aug 19, 2021

ollipa Aug 20, 2021

christophe-riolo Aug 20, 2021

ollipa Aug 20, 2021

adarshk7 Aug 20, 2021

ollipa Aug 21, 2021

christophe-riolo Aug 19, 2021

christophe-riolo Aug 19, 2021

ollipa Aug 21, 2021

christophe-riolo Aug 19, 2021

adarshk7 Aug 20, 2021

christophe-riolo Aug 19, 2021

ollipa Aug 20, 2021

adarshk7 Aug 20, 2021

ollipa left a comment

ollipa Aug 20, 2021

ollipa Aug 20, 2021

ollipa Aug 20, 2021

ollipa Aug 20, 2021

adarshk7 Aug 20, 2021

ollipa Aug 20, 2021

ollipa Aug 20, 2021

ollipa Aug 20, 2021

ollipa Aug 20, 2021

christophe-riolo commented Jan 11, 2022

ollipa commented Jan 12, 2022

ollipa commented Nov 12, 2022

	expected = return_values[0].raises
	expected_exception = return_values[0].raises

Pylint fixes #59

Pylint fixes #59

Conversation

adarshk7 commented Aug 19, 2021 • edited

codecov-commenter commented Aug 19, 2021 • edited

Codecov Report

christophe-riolo 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ollipa 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christophe-riolo commented Jan 11, 2022

ollipa commented Jan 12, 2022

ollipa commented Nov 12, 2022

adarshk7 commented Aug 19, 2021 •

edited

codecov-commenter commented Aug 19, 2021 •

edited