-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
[MRG+2] DOC add links to github sourcecode in API reference #2777
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
Conversation
I'm a fan of this.. There was some discussion on this idea a while back on #1680 but no consensus was reached.. 👍 |
All reactions
Sorry, something went wrong.
What do you think of making the compilation specific to a git checkout, and And I wouldn't mind if this was turned into a generic sphinx extension, but On 22 January 2014 03:17, Jaques Grobler notifications@github.com wrote:
|
All reactions
Sorry, something went wrong.
Advantages of using
|
All reactions
Sorry, something went wrong.
I'd be much more favorable to using inspect rather than grep. I'd also say that in a first time we could punt on the *.pyx. |
All reactions
Sorry, something went wrong.
I've updated with links for classes, functions and methods in Python (but not Cython), using I've also changed from using |
All reactions
Sorry, something went wrong.
1 similar comment
I've updated with links for classes, functions and methods in Python (but not Cython), using I've also changed from using |
All reactions
Sorry, something went wrong.
I've now noticed scipy has similar, at https://github.com/scipy/scipy/blob/6a4460f68315f0669604054be91ceeacd606f0b6/doc/source/conf.py#L314. I need to look closer to see what cases they handle that we don't, etc. Since they explicitly version their dev builds with git revisions, they are able to use different commit references in the URL when for dev or release (has or tag name respectively). |
All reactions
Sorry, something went wrong.
Hey Joel, I think I'll let you have z look at scipy before I review this PR. |
All reactions
Sorry, something went wrong.
Diffs:
|
All reactions
Sorry, something went wrong.
@GaelVaroquaux, I think we should pull this in, and see if there are any problems with the current commit refs. |
All reactions
Sorry, something went wrong.
I'll have a look. I however have a hard time feeling excited about this feature. For me it is additional code that will at some point fail and require debugging or evolution (all code does) for little feature. I satisfy my need to look at source code by using "symbol??" in IPython. Sorry, I should be thankful for the feature and it's not much code actually. It's just that I have the feeling that we are quite heavily loaded in terms of maintenance and I personally never have time to push features that I really need. So feel like focusing on the high-value code, where of course high value varies from one person to another. :) |
All reactions
Sorry, something went wrong.
return | ||
|
||
try: | ||
revision = linkcode_resolve.revision |
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 am a bit dense, but where is this set?
Sorry, something went wrong.
All reactions
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.
In a previous version of the code, apparently :s
Sorry, something went wrong.
All reactions
Sure, I understand that. I prefer On 13 February 2014 08:54, Gael Varoquaux notifications@github.com wrote:
|
All reactions
Sorry, something went wrong.
'blob/{revision}/{pre_path}{path}#L{lineno}') | ||
|
||
|
||
def linkcode_resolve(domain, info): |
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 that you should indicate (eg in a comment, or in the docstring) that this is used by the sphinx linkcode extension.
Sorry, something went wrong.
All reactions
Lightweight and elegant code. I made a couple of comment. My only additional and non trivial comments would be:
Thanks a lot for your work, as usual. |
All reactions
Sorry, something went wrong.
I've recently moved to a new computer and it seems I can't even run I don't think there'll be a problem testing sphinxext if we want to. On 13 February 2014 09:16, Coveralls notifications@github.com wrote:
|
All reactions
Sorry, something went wrong.
The refactored version can now be more easily tested, e.g. using standard library or sphinx modules for independence: >>> from github_link import _linkcode_resolve
>>> _linkcode_resolve('py', {'module': 'distutils', 'fullname': 'config.PyPIRCCommand.initialize_options'},
... package='distutils',
... url_fmt='http://hg.python.org/cpython/file/{revision}/Lib/{package}/{path}#L{lineno}',
... revision='xxxx')
'http://hg.python.org/cpython/file/xxxx/Lib/distutils/config.py#L105' |
All reactions
Sorry, something went wrong.
Getting an error building the docs with
I am also getting an error with nosetests, in the doctest part
I am using Python 3.4, other packages are from Anaconda 2.0 - looks like the first might be a Py3 strings issue, specifically buffer vs. memoryview? As for the second, @ogrisel mentioned a possible issue with local imports in python 3? |
All reactions
Sorry, something went wrong.
In Python 2.7 I only get the Additionally, the docs that are built work fine locally (well, as good as master, which still has a few issues it seems).
After talking with @ogrisel, it seems this is happening because of the rebase with master - since rebase changed the commit hash the link doesn't show up correctly. It should work fine once merged, beyond the |
All reactions
Sorry, something went wrong.
@fabianp I'll revert it and see what happens. |
All reactions
Sorry, something went wrong.
I can't understand the AppVeyor interface - it seems to be missing the consoles that I usually go to for explanation of the failure. |
All reactions
Sorry, something went wrong.
Yes there is a momentary bug. I reported it: http://help.appveyor.com/discussions/problems/541-job-that-runs-untill-timeout-without-any-output |
All reactions
Sorry, something went wrong.
I will disable appveyor in the mean time. |
All reactions
Sorry, something went wrong.
I've done with revert, forcing a Travis check by rebasing. |
All reactions
Sorry, something went wrong.
Failing with
|
All reactions
Sorry, something went wrong.
from functools import partial | ||
|
||
|
||
def _get_git_revision(CMD='git rev-parse --short HEAD'): |
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'm not crazy about this function definition. I can't think of any context where you'd want to call this function with an argument, and argument names shouldn't be all caps.
I'd make CMD=... a variable defined within the function.
Sorry, something went wrong.
All reactions
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.
goes to current head, so everything is fine.
Sorry, something went wrong.
All reactions
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 just commenting about the style.
Sorry, something went wrong.
All reactions
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.
Sometimes default args are used as a not-quite-global way to store constants (which was more relevant when github_link
wasn't its own module). However, I may have also done this because one may choose to use --abrev-ref
(which shows the branch name) instead of --short
(which shows the SHA). I'll change it to a global constant.
Sorry, something went wrong.
All reactions
Ubuntu 14.04 LTS, Python3.4, NumPy 1.8.1: I get the same error, @kastnerkyle , on this branch, but not on master |
All reactions
Sorry, something went wrong.
Found and fixed, @kastnerkyle. |
All reactions
Sorry, something went wrong.
It builds fine for me now - the only thing I still see is
because I am offline right now on my docbuild machine. This seems fine to me but just wanted to report it. Other than that, this is 👍 for me |
All reactions
Sorry, something went wrong.
That's actually a different matter. It's referring to links to numpy and On 21 July 2014 21:21, Kyle Kastner notifications@github.com wrote:
|
All reactions
Sorry, something went wrong.
OK great. I am usually online all the time - my connection bumped sometime during the doc build I guess. This seems like a really nice feature for usability and browsing! |
All reactions
Sorry, something went wrong.
I am building the doc from this PR on my box as well. Will let you know when it's done. |
All reactions
Sorry, something went wrong.
Under Python 3.4 I get URL such as: https://github.com/scikit-learn/scikit-learn/blob/b%2779afc76%27/sklearn/ensemble/forest.py#L1116 The git hash should be decode with the |
All reactions
Sorry, something went wrong.
Good catch - I didn't notice cause I have no internet :) On Mon, Jul 21, 2014 at 2:01 PM, Olivier Grisel notifications@github.com
|
All reactions
Sorry, something went wrong.
If I fix the URL manually to remobe the leading |
All reactions
Sorry, something went wrong.
'blob/{revision}/{package}/' | ||
'{path}#L{lineno}') | ||
""" | ||
revision = _get_git_revision() |
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.
revision = _get_git_revision().decode('ascii')
for instance.
Sorry, something went wrong.
All reactions
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.
Actually it's probably better to fold the decode call into the _get_git_revision
function itself.
Sorry, something went wrong.
All reactions
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.
Thanks @ogrisel
Sorry, something went wrong.
All reactions
I've squashed the commit history. |
All reactions
Sorry, something went wrong.
Before your pushed 060c248, I launched a full make clean html under Python 3 with the following change:
and I confirm that it works as expected. I have not checked with Python 2 though. |
All reactions
Sorry, something went wrong.
I'm now generating the URL as a |
All reactions
Sorry, something went wrong.
Changed to produce a unicode string. |
All reactions
Sorry, something went wrong.
So I think we should always decode the bytes to generate URL that is a unicode string, that is |
All reactions
Sorry, something went wrong.
Thanks for the fix. merging! |
All reactions
Sorry, something went wrong.
[MRG+2] DOC add links to github sourcecode in API reference
Great! I look forward to using this when introducing people to particular scikit-learn components. |
All reactions
Sorry, something went wrong.
Does it point on master or a particular branch? |
All reactions
Sorry, something went wrong.
A commit ref, to ensure line number match. It's live, e.g. On 22 July 2014 17:45, Arnaud Joly notifications@github.com wrote:
|
All reactions
Sorry, something went wrong.
Successfully merging this pull request may close these issues.
I enjoy using the API reference, but think a link to the source code on github would often help.
Becausesphinx.ext.linkcode
does not provide the full path or line number of the class/function, I'm getting this stuff using grep, which relies on a unix shell and certain path structures, which is a bit not-nice. Since github needs a branch/commit reference, I'm usinggit rev-parse --abbrev-ref HEAD
which can return a branch/tag name, but a more static commit reference (i.e.rev-parse --short
) might be more sensible.WDYT? ping @jaquesgrobler