Skip to content

Conversation

raghavrv
Copy link
Contributor

This is an attempt to sync between numpy's and scikit-learn's version of numpydoc.

Currently I have made numpydoc's version PEP8 compliant to avoid trivial merge conflicts between both versions.

@larsmans Could you take a look at this?

@@ -500,8 +509,8 @@ def __init__(self, cls, doc=None, modulename='', func_doc=FunctionDoc,
raise ValueError("Expected a class or None, but got %r" % cls)
self._cls = cls

self.show_inherited_members = config.get('show_inherited_class_members',
True)
self.show_inherited_members = \
Copy link
Member

Choose a reason for hiding this comment

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

no backslashes please, that's bad style. if this is copied from scikit-learn, it should be changed there. If you wanted to get this line to <80 chars, a better way would be

self.show_inherited_members = config.get(
            'show_inherited_class_members', True)

@rgommers
Copy link
Member

rgommers commented Apr 3, 2015

This looks fine except for the one \. Sounds like a good plan to merge back scikit-learn specific changes - they could probably use this repo as a git submodule just like numpy does.

@raghavrv
Copy link
Contributor Author

raghavrv commented Apr 4, 2015

@rgommers Have removed the \. Please take a look now!

they could probably use this repo as a git submodule just like numpy does.

Thanks! I have referenced your suggestion in a comment...

@rgommers
Copy link
Member

rgommers commented Apr 4, 2015

Looks good to me now, thanks. I'll leave it open for a bit in case someone else wants to comment.

@larsmans
Copy link
Contributor

larsmans commented Apr 9, 2015

Looks fine, but I'm not sure why you're pinging me. I don't have pull rights on this repo.

rgommers pushed a commit that referenced this pull request Apr 9, 2015
@rgommers rgommers merged commit 16ef04c into numpy:master Apr 9, 2015
@rgommers
Copy link
Member

rgommers commented Apr 9, 2015

OK in it goes. Thanks @ragv

@raghavrv
Copy link
Contributor Author

raghavrv commented Apr 9, 2015

Thanks for the merge :)

@raghavrv
Copy link
Contributor Author

raghavrv commented Apr 9, 2015

@larsmans Sorry I had incorrectly assumed you had! Thanks for reviewing it anyway :)

@raghavrv raghavrv deleted the scikit_learn_sync branch April 9, 2015 23:01
@rgommers rgommers added this to the v0.6 milestone Jan 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants