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

Make ScalarMappable a new-style class. #3653

Closed
wants to merge 1 commit into from

Conversation

switham
Copy link
Contributor

@switham switham commented Oct 17, 2014

(In cm.py:) class ScalarMappable: ==> class ScalarMappable(object):

There are no other old-style classes in cm.py, but, e.g.,

$ grep '^class' *.py |grep -v '('
__init__.py:class Verbose:
animation.py:class FFMpegBase:
animation.py:class MencoderBase:
animation.py:class ImageMagickBase:
artist.py:class ArtistInspector:
axis.py:class Ticker:
backend_bases.py:class GraphicsContextBase:
backend_bases.py:class Event:
backend_bases.py:class Cursors:
cbook.py:class CallbackRegistry:
cbook.py:class Bunch:
cbook.py:class Sorter:
cbook.py:class Null:
cbook.py:class GetRealpathAndStat:
cbook.py:class RingBuffer:
cbook.py:class MemoryMonitor:
contour.py:class ContourLabeler:
dates.py:class strpdate2num:
dates.py:class rrulewrapper:
figure.py:class SubplotParams:
fontconfig_pattern.py:class FontconfigPatternParser:
hatch.py:class HatchPatternBase:
lines.py:class VertexSelector:
mlab.py:class PCA:
mlab.py:class FIFOBuffer:
mlab.py:class FormatObj:
rcsetup.py:class ValidateInStrings:
rcsetup.py:class validate_nseq_float:
rcsetup.py:class validate_nseq_int:
rcsetup.py:class ValidateInterval:
sankey.py:class Sankey:
texmanager.py:class TexManager:
ticker.py:class Base:
units.py:class AxisInfo:
units.py:class ConversionInterface:
widgets.py:class LockDraw:
$

@tacaswell tacaswell added this to the v1.5.x milestone Oct 17, 2014
@tacaswell
Copy link
Member

We are near the start of the next 1.X release cycle, this is a great time to do a modernization effort like making all of the class new-style. Feel like doing all of them?

@switham
Copy link
Contributor Author

switham commented Oct 17, 2014

Well, honestly, not if that means reading all the code to find out whether any old-style class behavior is counted on. I don't particularly have an eye for that. I did see an example of Parentclass.method(self, *args), and I guess that's considered bad practice but would still work the same assuming there is only one Parentclass.

I'm a little confused why ScalarMappable is considered a "mixin" class. Any other old-style peculiarities to look for in matplotlib?

I just glanced through this: https://wiki.python.org/moin/NewClassVsClassicClass

Maybe I could just change all the class headers and see whether Travis CI likes it...?

@tacaswell
Copy link
Member

I don't think we have any code that relies on old-style specific behaviour. The same code runs on both 2 and 3 (where everything inherits from object) so I would just make the changes and see if Travis complains ;)

As for the explicitly calling super classes vs using super, if anything in the mro uses super then all classes in the mro need to use it (or calls won't propagate properly when they hit the non-cooperating class (see the fun of supporting both qt4 and qt5)). Moving the code base to use super would be a much more ambitious.

It is considered a mix-in because it is mostly (5 out of 6 times inherited from) used as one member of multiple inheritance.

@switham
Copy link
Contributor Author

switham commented Oct 17, 2014

I get it. Okay, I'm closing this PR and I will start another.

@switham switham closed this Oct 17, 2014
@tacaswell
Copy link
Member

github tracks branches not commits. You can just keep pushing new commits to this branch and they will all show up in this PR.

@switham
Copy link
Contributor Author

switham commented Oct 17, 2014

I deleted this one because I didn't see a way to rename the branch / pull request.

@tacaswell
Copy link
Member

you can rename the PR using the edit button at the top. Don't worry about the branch name too much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants