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

Jump to bottom

MNT Fix incorrect source code link for wrapped objects #17247

Merged
merged 1 commit into from May 17, 2020
Merged

MNT Fix incorrect source code link for wrapped objects #17247

merged 1 commit into from May 17, 2020

Conversation

alfaro96
Copy link
Member

Reference Issues/PRs

This closes #17242, and closes #10542.

What does this implement/fix? Explain your changes.

Fixes the source code link for objects wrapped by a decorator. Previously, the path to the file where the decorator is defined was returned.

alfaro96
alfaro96 commented May 15, 2020
View reviewed changes
doc/sphinxext/github_link.py
@@ -40,12 +40,13 @@ def _linkcode_resolve(domain, info, package, url_fmt, revision):
return

class_name = info['fullname'].split('.')[0]
if type(class_name) != str:
Copy link
Member Author

Choose a reason for hiding this comment

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

I have removed these lines because they were intended only for Python 2, and, as far as I am concerned, scikit-learn does not support Python 2 anymore.

@alfaro96
Copy link
Member Author

alfaro96 commented May 15, 2020 •
edited

@thomasjpfan Note that the source code link points to the decorator, instead of the function name. That's because inspect.getsourcelines considers the decorator as the first line of the function. I suppose that this is not an issue.

TomDLT
TomDLT reviewed May 16, 2020
View reviewed changes
Copy link
Member

@TomDLT TomDLT left a comment

Choose a reason for hiding this comment

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

Nice, it seems to work (see f1_score in the build)!

@thomasjpfan thomasjpfan added this to the 0.23.1 milestone May 16, 2020
thomasjpfan
thomasjpfan approved these changes May 16, 2020
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment •
edited

Choose a reason for hiding this comment

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

LGTM

We can do slightly more fancy things to get the lineno to match up, but I think this PR works well as is.

@alfaro96
Copy link
Member Author

alfaro96 commented May 16, 2020 •
edited

I agree with that. The exact line number could be obtained with some "hacks", but this will mess the code.

@thomasjpfan thomasjpfan changed the title [MRG] Fix incorrect source code link for wrapped objects MNT Fix incorrect source code link for wrapped objects May 17, 2020
@thomasjpfan thomasjpfan merged commit 0ba19ad into scikit-learn:master May 17, 2020
@thomasjpfan
Copy link
Member

Thank you @alfaro96 !

@thomasjpfan thomasjpfan mentioned this pull request May 17, 2020
Closed
@rth
Copy link
Member

rth commented May 18, 2020 •
edited

@thomasjpfan @adrinjalali Should we cherry pick this to 0.23.X already now without waiting for the rest of bug fixes? As users keeps hitting this, and there are already 3 opened issues.

@adrinjalali
Copy link
Member

AFAIK there aren't any left that we're waiting for. I'm creating the release PR now.

adrinjalali pushed a commit to adrinjalali/scikit-learn that referenced this pull request May 18, 2020
@alfaro96 @adrinjalali
MNT Fix incorrect source code link for wrapped objects (scikit-learn#…
b5e8495 
…17247)
adrinjalali pushed a commit that referenced this pull request May 19, 2020
@alfaro96 @adrinjalali
MNT Fix incorrect source code link for wrapped objects (#17247)
c19aa54
@alfaro96 alfaro96 deleted the fix_link_to_source_code branch May 20, 2020 07:32
viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020
@alfaro96 @viclafargue
MNT Fix incorrect source code link for wrapped objects (scikit-learn#…
a4465af 
…17247)
@hasan-yaman hasan-yaman mentioned this pull request Jun 4, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TomDLT TomDLT TomDLT left review comments

@thomasjpfan thomasjpfan thomasjpfan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.23.1
Development

Successfully merging this pull request may close these issues.

Fix incorrect source code link Documentation for decorated functions has incorrect source code links
5 participants
@alfaro96 @thomasjpfan @rth @adrinjalali @TomDLT