Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pylint fixes #59
Pylint fixes #59
Changes from all commits
33a251f
49153b4
b1cfb5a
17df468
183a411
4f29c41
7d6da94
ec21974
bb752bc
c77e604
2c52c4f
15a3858
bdd6e47
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can be smart with itertools also:
(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 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit hard to understand what is happening. Can you add some comments for explanation and unpack the code a bit? It's very concise now and I think it makes it harder to read.
We could also have only the function call in the try block:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what was the need for
_getattr
, I hope we are not breaking anything there … Have you looked up the history ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we wouldn't need the
_getattr
function since it's basically just an alias ofobject.__getattribute__
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we can get rid of the function and call
object.__getattribute__
. However, if we changeExpectation.__getattribute__
like I suggested, then there is no need to callobject.__getattribute__
(expect insideExpectation.__getattribute__
itself).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is different from the existing behavior. If
return_values
is empty, herereturn_values
would be[ReturnValue()]
and in the existing code it is empty (ReturnValue()
is never appended toreturn_values
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually you can just return here early if return_values is empty:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually not, because if
expectation._return_value
is empty, then the (mutable) reference to it is not what gets assigned to thereturn_values
variable, soexpectation._return_value
would not be mutated at all in the case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right. Very confusing 😁 Anyway I think early return when
return_values
is empty would make this better.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a new method to
FlexmockContainer
that does this? For exampleFlexmockContainer.saved_expectation
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to do a copy ? You are not modifying the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's needed because
Exceptation.reset()
callsdel self
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that not already a boolean ? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is but I guess it was done this way so that pylint doesn't complain about not using the ternary operator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can again use
contextlib.suppress
here, it would look cleaner