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.

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

Use more Pythonic super().__init__() instead of calling parent class directly #2198

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@jdufresne
Copy link
Contributor

commented Nov 5, 2016

No description provided.

@wiredfool

This comment has been minimized.

Copy link
Member

commented Nov 7, 2016

I'm not clear that super(Class, instance) is any more pythonic than the explicit parent call, at least in 2.x.

  1. it replaces an explicit MRO with an implicit MRO controlled by the child.
  2. It's ugly.
@jdufresne

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2016

Take a look at Raymond Hettinger's article "Python’s super() considered super! ".

it replaces an explicit MRO with an implicit MRO controlled by the child.

The MRO is still explicit. It is defined on the class line. The MRO and super() use a deterministic algorithm that, once understood, should not introduce surprises.

It's ugly.

I suppose that is subjective. As someone who uses super() regularly, I don't feel this way. But if the Pillow project has a coding style regarding super(), so be it.

@wiredfool

This comment has been minimized.

Copy link
Member

commented Nov 8, 2016

The MRO is still explicit. It is defined on the class line. The MRO and super() use a deterministic >algorithm that, once understood, should not introduce surprises.

The MRO of super() is controlled at runtime, the MRO of the explicit parent call is determined when the code is authored. Those are two different things. This is covered in super considered super.

@jdufresne jdufresne force-pushed the jdufresne:super branch from d83c77e to 1c5d205 Nov 11, 2016

@jleclanche

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2017

Yes please, this really should be merged.

@wiredfool

This comment has been minimized.

Copy link
Member

commented Jan 6, 2017

I'm still -1, given the ugly syntax required on 2.7 and the fact that a child can mess with a parent's method resolution order with super.

@jleclanche

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2017

@wiredfool that's exactly the point. When subclassing PIL classes, one expects those PIL classes to be calling super() appropriately.

@jdufresne

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2017

This was tagged as "Needs Rebase" by @radarhere , but was also rejected by @wiredfool. If I put in the work to rebase this will it be reconsidered for inclusion?

@radarhere

This comment has been minimized.

Copy link
Member

commented Apr 19, 2017

If you don't feel inspired to rebase, then I wouldn't recommend doing so, particularly with an issue like this, where there is not a lot of nuance in the implementation.

@wiredfool

This comment has been minimized.

Copy link
Member

commented Apr 19, 2017

I think the tag was essentially a mechanical marking of all the PRs with merge errors.

@homm

This comment has been minimized.

Copy link
Member

commented Jul 9, 2017

@wiredfool

it replaces an explicit MRO with an implicit MRO controlled by the child.

Certainly, but this is desired behaviour. Python is very flexible language and there should be a way to insert your class in the middle of MRO. Super allows this.
As of ugly syntax, you are definitely right, but this it the thing we all have to put up with for a compatibility.

@jdufresne

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2017

If you don't feel inspired to rebase, then I wouldn't recommend doing so, particularly with an issue like this, where there is not a lot of nuance in the implementation.

I have no problem doing the work, I like contributing. So I feel plenty inspired. I just don't want to waste everyone's time (including my own) if it is known ahead of time that it will be rejected. FWIW, I still think using super() is preferable.

@jdufresne jdufresne force-pushed the jdufresne:super branch 4 times, most recently from 5a8dd11 to c9a83c2 Dec 16, 2017

@radarhere radarhere removed the Needs Rebase label Dec 16, 2017

@jdufresne jdufresne force-pushed the jdufresne:super branch 2 times, most recently from ba3eea2 to 48a0aeb Jan 1, 2018

@jdufresne

This comment has been minimized.

Copy link
Contributor Author

commented Jan 1, 2018

Rebased to latest master.

@radarhere radarhere removed the Needs Rebase label Jan 1, 2018

@jdufresne jdufresne force-pushed the jdufresne:super branch from 48a0aeb to 6af855e Jan 5, 2018

@jdufresne jdufresne force-pushed the jdufresne:super branch from 6af855e to 7dd34c5 Apr 4, 2018

@codecov

This comment has been minimized.

Copy link

commented Apr 4, 2018

Codecov Report

Merging #2198 into master will decrease coverage by 2.61%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2198      +/-   ##
=========================================
- Coverage   83.21%   80.6%   -2.62%     
=========================================
  Files         169     164       -5     
  Lines       23507   22493    -1014     
  Branches     2795    2795              
=========================================
- Hits        19562   18130    -1432     
- Misses       3945    4363     +418
Impacted Files Coverage Δ
src/PIL/ImageWin.py 60.22% <0%> (ø) ⬆️
src/PIL/ImageTk.py 20.31% <0%> (-54.69%) ⬇️
src/PIL/ImageQt.py 22.77% <0%> (-60.4%) ⬇️
src/PIL/TarIO.py 89.47% <100%> (ø) ⬆️
src/PIL/PngImagePlugin.py 85.96% <100%> (-4.21%) ⬇️
src/PIL/TiffImagePlugin.py 81.16% <100%> (-8.45%) ⬇️
src/PIL/ImageFile.py 85.49% <100%> (-5.75%) ⬇️
src/PIL/BdfFontFile.py 96.55% <100%> (ø) ⬆️
src/PIL/PcfFontFile.py 89.14% <100%> (ø) ⬆️
src/PIL/MicImagePlugin.py 4.65% <0%> (-93.03%) ⬇️
... and 54 more

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 ca850a6...7dd34c5. Read the comment docs.

@jdufresne jdufresne force-pushed the jdufresne:super branch from 7dd34c5 to 8675d92 Sep 5, 2018

@jdufresne jdufresne force-pushed the jdufresne:super branch from b394bd4 to 25adf77 Jan 27, 2019

@jdufresne jdufresne force-pushed the jdufresne:super branch from 25adf77 to f3e19f8 Mar 2, 2019

@jdufresne jdufresne force-pushed the jdufresne:super branch from f3e19f8 to 6f5a080 May 25, 2019

@jdufresne jdufresne force-pushed the jdufresne:super branch from 6f5a080 to 586e76a Jun 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.