Skip to content
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.

Sign up for GitHub

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

Added source code link #172

Merged
merged 9 commits into from
Oct 14, 2022
Merged

Added source code link #172

Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2cd3a1c
Update conf.py
ayyucedemirbas Oct 4, 2022
b3a1f4c
all done
ayyucedemirbas Oct 4, 2022
b59d98c
some minor changes
ayyucedemirbas Oct 4, 2022
fce3a1b
Redundant imports are removed
ayyucedemirbas Oct 4, 2022
5312b14
runned black and isort on the conf.py
ayyucedemirbas Oct 6, 2022
ff41416
All problems are solved
ayyucedemirbas Oct 13, 2022
7d643a2
Added domain check
ayyucedemirbas Oct 13, 2022
e9dca16
Update changes.rst
ayyucedemirbas Oct 13, 2022
e0e254b
Merge branch 'main' into main
BenjaminBossan Oct 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions docs/conf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@

# -- Path setup --------------------------------------------------------------

import inspect
import os
from operator import attrgetter

from packaging.version import parse

# If extensions (or modules to document with autodoc) are in another directory,
Expand Down Expand Up @@ -36,6 +40,7 @@
# extensions coming with Sphinx (named 'sphinx.ext.*') or your custom
# ones.
extensions = [
"sphinx.ext.linkcode",
"sphinx.ext.autodoc",
"numpydoc",
"sphinx_gallery.gen_gallery",
Expand Down Expand Up @@ -65,6 +70,46 @@
# (note that `group` is the GitHub user or organization)
issues_github_path = "skops-dev/skops"


def linkcode_resolve(domain, info):
if domain != "py":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adrin already mentioned this, but this line and the one below

if domain not in ("py", "pyx"):

do redundant work. Only the latter is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅ Done

return None
if not info["module"]:
return None
Copy link
Member

Choose a reason for hiding this comment

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

this is checked bellow again (info.get("module") is safer than info["module"] since the latter can raise an exception if the key doesn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅ Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it works, I updated the PR. Could you please take a look at it? Thank you so much for everything!

filename = info["module"].replace(".", "/")
Copy link
Collaborator

Choose a reason for hiding this comment

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

filename is not being used and can be deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅ Done

if domain not in ("py", "pyx"):
return
if not info.get("module") or not info.get("fullname"):
return

class_name = info["fullname"].split(".")[0]
module = __import__(info["module"], fromlist=[class_name])
obj = attrgetter(info["fullname"])(module)

# Unwrap the object to get the correct source
# file in case that is wrapped by a decorator
obj = inspect.unwrap(obj)

try:
fn = inspect.getsourcefile(inspect.unwrap(obj))
except TypeError:
try:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
try:
try:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅ Done

fn = inspect.getsourcefile(inspect.unwrap(obj.fget))
except (AttributeError, TypeError):
fn = None
if not fn:
return None
package = "skops"
fn = os.path.relpath(fn, start=os.path.dirname(__import__(package).__file__))
try:
lineno = inspect.getsourcelines(obj)[1]
except Exception:
lineno = ""
return (
"https://github.com/skops-dev/skops/blob/main/skops/" + fn + "#L" + str(lineno)
Copy link
Member

Choose a reason for hiding this comment

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

this shouldn't refer to the main branch, it should refer to the branch for which this documentation is being built. For example, in sklearn in this page for ClassifierMixin you get this link which is not on main.

Copy link
Contributor Author

@ayyucedemirbas ayyucedemirbas Oct 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I added these lines from scikit-learn:

REVISION_CMD = "git rev-parse --short HEAD"

def _get_git_revision():
    try:
        revision = subprocess.check_output(REVISION_CMD.split()).strip()
    except (subprocess.CalledProcessError, OSError):
        print("Failed to execute git to get revision")
        return None
    return revision.decode("utf-8")

def linkcode_resolve(domain, info):
   ...
    revision = _get_git_revision()
    if revision is None:
        return
   ...
    return (
        "https://github.com/skops-dev/skops/blob/{revision}/skops/" + fn + "#L" + str(lineno)
    )

However, the source code links became something like this (https://github.com/skops-dev/skops/blob/%7Brevision%7D/skops/hub_utils/_hf_hub.py#L328) and of course, they don't work

Copy link
Member

Choose a reason for hiding this comment

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

Could you try this patch:

diff --git a/docs/conf.py b/docs/conf.py
index 3ed773f..4ed5d3d 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -8,6 +8,7 @@
 
 import inspect
 import os
+import subprocess
 from operator import attrgetter
 
 from packaging.version import parse
@@ -71,6 +72,19 @@ autosummary_generate = True
 issues_github_path = "skops-dev/skops"
 
 
+# -- linkcode configuration --------------------------------------------------
+REVISION_CMD = "git rev-parse --short HEAD"
+
+
+def _get_git_revision():
+    try:
+        revision = subprocess.check_output(REVISION_CMD.split()).strip()
+    except (subprocess.CalledProcessError, OSError):
+        print("Failed to execute git to get revision")
+        return None
+    return revision.decode("utf-8")
+
+
 def linkcode_resolve(domain, info):
     if domain != "py":
         return None
@@ -93,7 +107,7 @@ def linkcode_resolve(domain, info):
     try:
         fn = inspect.getsourcefile(inspect.unwrap(obj))
     except TypeError:
-        try: 
+        try:
             fn = inspect.getsourcefile(inspect.unwrap(obj.fget))
         except (AttributeError, TypeError):
             fn = None
@@ -105,9 +119,16 @@ def linkcode_resolve(domain, info):
         lineno = inspect.getsourcelines(obj)[1]
     except Exception:
         lineno = ""
-    return (
-        "https://github.com/skops-dev/skops/blob/main/skops/" + fn + "#L" + str(lineno)
+    url_fmt = (
+        "https://github.com/scikit-learn/"
+        "scikit-learn/blob/{revision}/"
+        "{package}/{path}#L{lineno}"
     )
+    revision = _get_git_revision()
+    return url_fmt.format(revision=revision, package=package, path=fn, lineno=lineno)
+
+
+# -- linkcode configuration --------------------------------------------------
 
 
 # -- Options for HTML output -------------------------------------------------

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Returns something like this: https://github.com/scikit-learn/scikit-learn/blob/a6db88f/skops/hub_utils/_hf_hub.py#L594
and also doesn't work

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are two problems why this doesn't work. First of all it links to sklearn but we want to link to skops (I guess Adrin forgot to adjust the copied code):

    url_fmt = (
        "https://github.com/skops-dev/"
        "skops/blob/{revision}/"
        "{package}/{path}#L{lineno}"
    )

But even after that, the link wouldn't lead anywhere. However, this is probably because the git revision's hash does not lead anywhere (yet). If we replace it with the hash of an actual commit (current HEAD on main branch), it will work, e.g.:

https://github.com/skops-dev/skops/blob/69621cd/skops/hub_utils/_hf_hub.py#L594

where 69621cd is the current git rev-parse --short HEAD. So when you test the link locally, it might not work because the commit doesn't exist on GitHub. But that's no problem, once the code is merged, the links will be correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahahahahhahahhahah I feel like an idiot. Thank you so much!

)


# -- Options for HTML output -------------------------------------------------

# The theme to use for HTML and HTML Help pages. See the documentation for
Expand Down