Skip to content

Conversation

maniteja123
Copy link
Contributor

Added Raises section to utils.validation. This was an idea by @rvraghav93 in #6198 which seemed helpful. If feasible can do it for other modules too. Otherwise, please close the PR.

@GaelVaroquaux GaelVaroquaux changed the title DOC: Add Raises Section [MRG+1] DOC: Add Raises Section Jan 21, 2016
@GaelVaroquaux
Copy link
Member

Looks good in general. This does beg the question of how to maintain this information. However, the question applies to any type of docstring, so it shouldn't deter us from merging this.

You have my +1

@maniteja123
Copy link
Contributor Author

In the first attempt, it felt that at automated way of finding the raised exceptions by the method would require knowing all the functions used by this method. Somewhat similar to the way parameters of a function are extracted.

That said it would be really difficult IMO to take care of recursive calls and parameters passed through those calls to the functions since the exceptions raised are also dependent on them.

Regarding the maintainability, probably these should be added right during the addition of the feature to the code base as a practice. And also maybe we can look at some other softwares following these kind of documentation.

These are just my opinions and definitely devs are the best people to comment on this issue.

Cheers.

@raghavrv
Copy link
Member

at automated way of finding the raised exceptions

Ummm I don't think we can do that.

The simple way would be to search for .*Error in that function and manually list them.

@raghavrv
Copy link
Member

probably these should be added right during the addition of the feature to the code base as a practice

yes.

@maniteja123
Copy link
Contributor Author

Thanks for all the comments. I understand it is really an additional burden to maintain this and might also add to the lines without actually helping much. But if it is okay, will proceed on adding similarly to other function too ? Thanks.

@raghavrv
Copy link
Member

Please confirm with @GaelVaroquaux at #6198 before proceeding

TypeError
If the estimator is not an ``Estimator`` instance

_NotFittedError
Copy link
Member

Choose a reason for hiding this comment

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

Wait, that's private? Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry got confused with the underscore.

@amueller
Copy link
Member

these are all in private functions, so you didn't add anything to the rendered docs btw ....

@maniteja123
Copy link
Contributor Author

Thanks but I knew that they wouldn't be rendered in the docs. I just thought that it would be helpful to better understand the util functions and also would give an idea on how feasible and useful would this extra section be. Please let me know if you feel this is not an useful addition, will proceed accordingly.

@maniteja123
Copy link
Contributor Author

Thanks for all the suggestion. Will close this PR for now but will push the changes to this branch in case someone wants to look at it.

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.

4 participants