[MRG] Added tips for reading the code base #12874
Conversation
There was a problem hiding this comment.
Thanks @NicolasHug! I really like this! We should maybe add a bullet about class inheritance and say that some methods might be implemented in a parent class. And also talk about Mixins depending on the type of the estimator (classifier, regressor, clusterer or outlier detector).
| on GitHub. ``git grep`` (`examples | ||
| <https://git-scm.com/docs/git-grep#_examples>`_) is also extremely | ||
| useful to see every occurrence of a pattern (e.g. a function call or a | ||
| variable) in the code base. |
There was a problem hiding this comment.
Maybe add that git grep only searches files tracked by git by default compared to grep. Unless scikit-learn doc is not considered to be the place to say this. It's just that I have been using grep for a while before realizing that git grep might be better for this purpose :)
There was a problem hiding this comment.
You sure? I feel it's not really necessary since it's quiet unlikely that the intended audience of this section will ever need to look for patterns in untracked files, and if they do they can still refer to the doc in the link.
There was a problem hiding this comment.
Okay leave it as it is.
There was a problem hiding this comment.
Generally LGTM, thanks @NicolasHug
Maybe this deserves a link in CONTRIBUTING.md
|
Awesome, thanks! |
doc/developers/contributing.rst
Outdated
| default behaviour depending on the nature of the estimator (classifier, | ||
| regressor, transformer, etc.). | ||
| - Sometimes, reading the tests for a given function will give you an idea of | ||
| what is its intended purpose. You can use ``git grep`` (see below) to find |
There was a problem hiding this comment.
nitpick: -> what its intended purpose is.
doc/developers/contributing.rst
Outdated
| - Due to the use of `Inheritance | ||
| <https://en.wikipedia.org/wiki/Inheritance_(object-oriented_programming)>`_, | ||
| some methods may be implemented in parent classes. All estimators inherit | ||
| at least from ``BaseEstimator``, and from a ``Mixin`` class that enables |
There was a problem hiding this comment.
probably useful to link to the actual classes like you do with the LinearRegression above.
* Added tips for reading the code base * Put it in contributing.rst * Added bullet point about inheritance
* Added tips for reading the code base * Put it in contributing.rst * Added bullet point about inheritance
* Added tips for reading the code base * Put it in contributing.rst * Added bullet point about inheritance
This reverts commit a3748d2.
This reverts commit a3748d2.
* Added tips for reading the code base * Put it in contributing.rst * Added bullet point about inheritance
Reference Issues/PRs
Closes #12869
What does this implement/fix? Explain your changes.
This PR adds a section in
contributing.rstwith tips to be more efficient at reading/understanding the codeAny other comments?
Feel free to add / change anything you deem relevant