Skip to content

Coding standards super() encouraged - #6

Merged
eteq merged 7 commits into
astropy:masterfrom
embray:coding-standards-super
Aug 23, 2011
Merged

eteq merged 7 commits into
astropy:masterfrom
embray:coding-standards-super

Conversation

@embray

@embray embray commented Aug 18, 2011

Copy link
Copy Markdown
Member

Here are my updates to the Coding Standards documented explaining why use of super() ought to be encouraged rather than discouraged. It also adds a requirement that all new classes must be new-style classes.

It also adds .swp files to the .gitignore, otherwise I'd go crazy :)

embray added 4 commits August 17, 2011 15:05
…iscouragment of super() use to encouragement.
…ut with

many of the semantics reversed.  super() good!  No super() bad!  Inconsistent
super() broken!
…ith the second example. Moved the Note on super pros/cons
@eteq

eteq commented Aug 18, 2011

Copy link
Copy Markdown
Member

Looks great to me... Thanks!

It might also make sense to add (either in the same bullet point or one right next to it) a statement discouraging multiple inheritance unless there's a good reason for it, as we discussed on-list (with a mixin example for "when it's ok"). If you want to add that in this or another pull request, that's fine, or I can try writing one when I get around to it. Just let me know which.

@embray

embray commented Aug 18, 2011

Copy link
Copy Markdown
Member Author

You're right--I thought of putting in something like that, but I wasn't sure where it would fit. But I'll figure something out and add it to this.

@embray

embray commented Aug 19, 2011

Copy link
Copy Markdown
Member Author

Updated with a multiple inheritance example. It's pretty abstract, but I didn't want the example to implement any specific functionality that would distract from the main point of how that mixin works.

@eteq

eteq commented Aug 20, 2011

Copy link
Copy Markdown
Member

The guideline looks great aside from the small suggestion I made inline.

Regarding the example, I definitely like the idea there, but I think it would be better to use MutableCollection instead of DictMixin. We are specifically saying 2.6 compatibility is required, so we should encourage people to make use of the 2.6 collections instead of the older styles. I think the only change you need to make is replace keys with __iter__, and add __len__ and __delitem__ methods, and you can point out that before 2.6 the same thing was done with DictMixin.

If it isn't too much trouble, it would also be good to have the example method be implemented rather than have all the functionality be "..." - I personally tend to get more out of examples that are runnable, even if the functionality is completely trivial (e.g. something that ends up just like a regular dict but prints messages or something).

@embray

embray commented Aug 22, 2011

Copy link
Copy Markdown
Member Author

I'll take out the background information on the C3 algorithm altogether--you're right that it's just distracting. I'll leave in the link which contains much of that same information anyways.

I also agree on changing DictMixin to MutableMapping, though I'm a bit iffy on filling out the rest of the example. The problem is that most of the use cases for using this in a multiple-inheritance context tend to be non-trivial. Your suggestion of having it print messages, for example, can be achieved simply my subclassing dict; no multiple-inheritance necessary. Maybe I'll try to come up with a different example, or even remove the example altogether and suggest "You don't need to use this unless you know you need to use this" though that feels like a bit of a cop-out.

embray added 2 commits August 22, 2011 11:41
… on MutableMapping it was becoming more of a mini-tutorial on ABCs than a mixin example. Best to leave it out and not encourage people to use multiple-inheritance at all unless they know it's the best solution to their problem.
@eteq

eteq commented Aug 23, 2011

Copy link
Copy Markdown
Member

Should I merge this now, then? Or are you still thinking you might want to write a slightly different mixin example?

@embray

embray commented Aug 23, 2011

Copy link
Copy Markdown
Member Author

No, my preference at this point is to not have an example at all. As far as I'm concerned it's ready to merge.

eteq added a commit that referenced this pull request Aug 23, 2011
Adds coding guideline indicating that super() is the best way to call superclass methods, and discouraging multiple inheritance.
@eteq
eteq merged commit a350d17 into astropy:master Aug 23, 2011
astrofrog added a commit that referenced this pull request Oct 26, 2011
updates for config/data plans
mdboom added a commit that referenced this pull request Sep 24, 2012
Move the edit-on-github link for API objects to be next to the [source] ...
eteq added a commit that referenced this pull request Dec 6, 2012
Modified approach for time init additions
keflavich referenced this pull request in keflavich/astropy Oct 9, 2013
Adds coding guideline indicating that super() is the best way to call superclass methods, and discouraging multiple inheritance.
keflavich referenced this pull request in keflavich/astropy Oct 9, 2013
updates for config/data plans
keflavich referenced this pull request in keflavich/astropy Oct 9, 2013
Move the edit-on-github link for API objects to be next to the [source] ...
keflavich referenced this pull request in keflavich/astropy Oct 9, 2013
Modified approach for time init additions
kelle pushed a commit to kelle/astropy that referenced this pull request Mar 25, 2016
nden pushed a commit to nden/astropy that referenced this pull request Jul 19, 2019
nstarman referenced this pull request in nstarman/astropy Aug 31, 2020
Co-authored-by: Adrian Price-Whelan <[email protected]>
jeffjennings pushed a commit to jeffjennings/astropy that referenced this pull request Jul 2, 2025
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.

2 participants