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 Super instead of explicit class naming #129

Closed
wants to merge 4 commits into from

Conversation

lukasjuhrich
Copy link
Contributor

Added some docstrings where appropriate as well.

Fixes #127

In this case, we can just use the bound method `self.__next__` instead
of calling the unbound method of `type(self)` and giving `self` as the
first parameter.
@lukasjuhrich
Copy link
Contributor Author

Oh, I should probably take a look at the other files, too.

@PierreSelim
Copy link
Contributor

Explicit is better than implicit.

Just saying :)

@lukasjuhrich
Copy link
Contributor Author

@PierreSelim I know that quite well.

But by the same argumentation you should never refactor out common constants because the that would be “implicit” and thus “bad”.

That's not the case here. The reason, at the positions I replaced it, the parent class has been used is because it was the parent class as given above. Therefore, it is quite explicit and to-the-point using a language construct with exactly that semantic meaning and use case.

I want the parent class. Which is not necessarily List in the future, but always what's been given above in the class definition.

This makes the code more reusable and easier changeable.

@lukasjuhrich lukasjuhrich mentioned this pull request Aug 11, 2016
@danmichaelo
Copy link
Member

I think the reason super wasn't used was just for historical reasons, I don't think it was around in really old days. Thanks for fixing, this has been merged.

When it comes to docstrings, it seems like I hadn't documented it, but we sort-of settled on using the “Google style”, which isn't very strictly defined, but exemplified here. It's quite readable in the source code, and can also be parsed by the napoleon sphinx extension when producing html documentation (example: http://mwclient.readthedocs.io/en/latest/reference/site.html)

I added a note about it to the readme: 29ef67b

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

3 participants