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.

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

Point [SOURCE] documents to github #20673

Merged
merged 1 commit into from
Aug 6, 2021
Merged

Conversation

dmatos2012
Copy link
Contributor

@dmatos2012 dmatos2012 commented Jul 19, 2021

PR Summary

Addresses #20557 to point the [SOURCE] in docs point to github instead. A proper build config to build the old way is still missing, but perhaps that can be part of separate PR. Also, this impl was copied from scipy conf.py, but modified slightly to work here instead.

PS: Building old way is just adding back the sphinx.ext.viewcode, but I was not sure how to add it as part of a make O=-Dmyarg=1 html.

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@jklymak
Copy link
Member

jklymak commented Jul 19, 2021

This looks good. I'm surprised that there ins't a built in sphinx option for this though....

doc/conf.py Outdated Show resolved Hide resolved
doc/conf.py Outdated Show resolved Hide resolved
@dmatos2012
Copy link
Contributor Author

dmatos2012 commented Jul 25, 2021

Sorry for the delay, was caught up with the non-digital world. I have addressed the comments by @QuLogic. I think now it should be fine hopefully, otherwise I can make changes np. @jklymak

@jklymak
Copy link
Member

jklymak commented Jul 25, 2021

Many of these links don't seem to go to the right place, though some of them do...

@dmatos2012
Copy link
Contributor Author

Are these the ones that are deprecated? While I tested it, I saw those would go to the deprecation github page. Aside from that, also add_callback confuses the function with the docstring add_callback. Any other ones? In that case, I think its the inspect functions themselves, but yeah not sure.

@jklymak
Copy link
Member

jklymak commented Jul 25, 2021

I only did a spot check, but starting from https://62590-1385122-gh.circle-artifacts.com/0/doc/build/html/api/axes_api.html barh works, but bar does not. fill_between and fill_betweenx do not work, for me at least (Safari). i.e. fill_between links to: https://github.com/matplotlib/matplotlib/blob/300e9cdb/lib/matplotlib/__init__.py#L5259-L5263

@dmatos2012
Copy link
Contributor Author

dmatos2012 commented Jul 25, 2021

So now those ones you spotted are fixed. I also checked around ~20 more of those, and they look ok as well.

Edit: lib/mpl_toolkits links were not generated, now it is. I guess 3rd time the charm?

@jklymak
Copy link
Member

jklymak commented Jul 25, 2021

Great. Do you know why they were different?

@dmatos2012
Copy link
Contributor Author

I noticed inspect.getsourcefile(obj) was returning the filename to be lib/matplotlib/__init__.py instead of the python file name in which it was defined. Thus, I added fn.endswith('__init__.py') to get the filename like shown below and then it shows the proper filename.

if not fn or fn.endswith('__init__.py'):
    fn = inspect.getsourcefile(sys.modules[obj.__module__])

doc/conf.py Outdated Show resolved Hide resolved
doc/conf.py Outdated Show resolved Hide resolved
doc/conf.py Outdated Show resolved Hide resolved
doc/conf.py Outdated Show resolved Hide resolved
doc/conf.py Outdated Show resolved Hide resolved
doc/conf.py Outdated Show resolved Hide resolved
doc/conf.py Outdated Show resolved Hide resolved
@dmatos2012
Copy link
Contributor Author

@QuLogic I added some extra Exceptions and added the rest of your suggestions.

@jklymak jklymak marked this pull request as ready for review August 4, 2021 14:56
@QuLogic QuLogic merged commit e9b9f59 into matplotlib:master Aug 6, 2021
@QuLogic QuLogic added this to the v3.5.0 milestone Aug 6, 2021
@dmatos2012 dmatos2012 deleted the doclinks_gh branch August 8, 2021 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants