-
Notifications
You must be signed in to change notification settings - Fork 223
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
Docstring return statements #789
Docstring return statements #789
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #789 +/- ##
=======================================
Coverage 81.45% 81.45%
=======================================
Files 154 154
Lines 10048 10048
=======================================
Hits 8185 8185
Misses 1863 1863
|
.git-blame-ignore-revs
Outdated
@@ -0,0 +1,3 @@ | |||
894744537dc64f729a44f64fbcb58e6b3b5dc1aa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're squashing commits, there should only be one entry per PR. Can you check if it's the case that the hash of the first commit is used after the squash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually wondering about this and meant to check how it works, and it seems the above does not. It seems that we'll have to do two separate PRs one with the changes and one with the relevant commits in .git-blame-ignore-revs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or squash locally before merging so the PR only has one commit? Edit: on second though, you'd need to know the commit hash before it's created so probably not...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I could squash the PR commits to one and then add another commit before merging into master instead of squashing?
@@ -198,7 +198,7 @@ def permed_lsdds( | |||
|
|||
Returns | |||
------- | |||
Vector of B LSDD estimates for each permutation, H_lam_inv which may have been inferred, and optionally | |||
Vector of B LSDD estimates for each permutation, H_lam_inv which may have been inferred, and optionally \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these explicit line breaks actually needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't realise this, good spot @mauicv !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
``` | ||
|
||
- Returning an object which contains multiple attributes and each attribute is described individually. | ||
In this case the attribute name is written between single back-ticks and the type, if provided, would be written in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick:
and the type, if provided, would be written
Could we be a little more specific here? Not detailing types in the general case (since already done by sphinx-autodoc-typehints
), but giving types when multiple objects are contained in another object (i.e. a dict
), makes a lot of sense IMO. However, in the latter case, are we saying this should always be done? Or its optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're hinting that it's better to give types however it's not something we've done historically. I've opened an issue to change this across the codebase and then perhaps we can change the language here to be a little stronger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor nitpick (or question even), but otherwise LGTM!
what is this
Some return statements in alibi detect are formatted incorrectly. This PR fixes this issue #759. These changes are all taken from #748 which will be closed. As an example of the kind of thing this fixes compare this to this.
CONTRIBUTING.md
here