Skip to content
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.

Sign up for GitHub

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

Jump to bottom

bug: AttributeError when using ValidationMonitorMixin #248

Merged
merged 2 commits into from May 7, 2020
Merged

bug: AttributeError when using ValidationMonitorMixin #248

merged 2 commits into from May 7, 2020

Conversation

rennerocha
Copy link
Collaborator

This PR fixes #246 .

@rennerocha
Initialize attribute on ValidationMonitorMixin with default value ins…
b313d20 
…tead of relying on class constructor (that is never called).

This commit will avoid that when we define a custom monitor without providing a default value for correct_field_list_handling attribute to raise
an AttributeError.
@rennerocha
Fix docstring to relate to correct attribute name
d06b1a3
@rennerocha rennerocha added this to the 1.12.1 milestone May 6, 2020
@rennerocha rennerocha requested review from wRAR and Gallaecio May 6, 2020 14:33
@codecov
Copy link

codecov bot commented May 6, 2020 •
edited

Codecov Report

Merging #248 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #248      +/-   ##
==========================================
+ Coverage   71.65%   71.70%   +0.05%     
==========================================
  Files          67       67              
  Lines        2759     2757       -2     
  Branches      309      309              
==========================================
  Hits         1977     1977              
+ Misses        718      716       -2     
  Partials       64       64
Impacted Files Coverage Δ
spidermon/contrib/monitors/mixins/validation.py 90.68% <100.00%> (+1.11%) ⬆️

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 e379613...d06b1a3. Read the comment docs.

@rennerocha rennerocha changed the title Attribute error with validation mixin bug: AttributeError when using ValidationMonitorMixin May 6, 2020
@Gallaecio Gallaecio requested a review from ejulio May 6, 2020 15:12
@Gallaecio
Copy link
Member

Gallaecio commented May 6, 2020 •
edited

#246 sounds to me like a bad usage in the code using the mixin (defining __init__ without calling super().__init__), so I’m not sure it is a bug. But I guess the change results in better usability. So +0 from me.

@rennerocha
Copy link
Collaborator Author

#246 sounds to me like a bad usage in the code using the mixin (defining __init__ without calling super().__init__), so I’m not sure it is a bug. But I guess the change results in better usability. So +0 from me.

Mixin __init__ is never called so that is way we have this bug. I agree that this is not the right usage for this attribute anyway. Maybe in a next release we can get rid of it. For now, let's make the mixin usable.

ejulio
ejulio approved these changes May 7, 2020
View reviewed changes
Copy link
Contributor

@ejulio ejulio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just adding that, if it's a mixin, probably we shouldn't call super()
As of my understanding, it is not inheritance, but composition.
So, we should be using a mixin to compose something and avoid the coupling through inheritance

@Gallaecio
Copy link
Member

Gallaecio commented May 7, 2020 •
edited

I’m not talking about using super in the mixin, but in the classes using the mixin.

Whenever you inherit from a class, be it a mixin or something else, you are still inheriting from a class, and you must use super in your __init__. Otherwise, you are preventing the __init__ of parent classes to be executed.

This specific mixin can be modified not to use __init__, but that will not always be the case.

@rennerocha rennerocha merged commit 1871c82 into scrapinghub:master May 7, 2020
@rennerocha rennerocha deleted the attribute-error-with-validation-mixin branch May 7, 2020 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wRAR wRAR wRAR approved these changes

@ejulio ejulio ejulio approved these changes

@Gallaecio Gallaecio Awaiting requested review from Gallaecio

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.12.2
Development

Successfully merging this pull request may close these issues.

bug: AttributeError when using ValidationMonitorMixin
4 participants
@rennerocha @Gallaecio @wRAR @ejulio