astropy / photutils

Astropy package for source detection and photometry. Maintainer: @larrybradley
https://photutils.readthedocs.io
BSD 3-Clause "New" or "Revised" License
240 stars 134 forks source link

Source code link error #1026

Closed PrasannaJayanthi closed 2 months ago

PrasannaJayanthi commented 4 years ago

I was looking at the tutorials on the photutils website and there is an error in the source code link. There are two slashes instead of one. This is the current link "https://photutils.readthedocs.io/en/stable//background-1.py", it works if the link is this "https://photutils.readthedocs.io/en/stable/background-1.py". I am assuming the issue is the same for all the source code links.

bsipocz commented 4 years ago

Thanks for the report.

It's an interesting plot:: directive issue. Interestingly I don't see it in another package, but indeed it's persistently present in photutils.

@larrybradley @pllim - would you think that we can linkcheck for these internal docs links, too?

pllim commented 4 years ago

I am not familiar with the code here. Where is that code generated?

larrybradley commented 4 years ago

Those links are dynamically generated by Sphinx, specifically the sphinx.ext.viewcode extension. I just noticed that astropy turned off showing the source code by setting plot_html_show_source_link = False in docs/conf.py.

pllim commented 4 years ago

But where is it generated? Which RST file? 😬

larrybradley commented 4 years ago

You can pick any of them. It applies to all plot:: directives. @PrasannaJayanthi report specifically refers to https://github.com/astropy/photutils/blob/master/docs/background.rst

larrybradley commented 4 years ago

It's definitely an upstream issue. specutils has the same issue, e.g. try the "Source code" link above the first plot on this page: https://specutils.readthedocs.io/en/latest/

pllim commented 4 years ago

Yeah, definitely, I noticed that in synphot as well. 🤔

bsipocz commented 4 years ago

Yes, it's certainly upstream, but I don't yet see where it's coming from. Weirdly I don't see it with astroML while messing with the new docs here: https://epyc.astro.washington.edu/~bsipocz/astroml/regression/_build/html/user_guide/regression.html (while it's annoying there, that I also get the "source code" links for code snippets even though I didn't asked for it.)

larrybradley commented 4 years ago

Hmm, maybe it's in sphinx-astropy? 🤔

bsipocz commented 4 years ago

Good point, though I don't yet see how, as we don't patch the plot directive there.

maybe @astrofrog has insights.

pllim commented 4 years ago

I don't see how this can add extra slash...

https://github.com/astropy/sphinx-astropy/blob/088f073519a02bd02a9c38dc8b14d81dcc4e69ad/sphinx_astropy/conf/v1.py#L138-L140

try:
    import matplotlib.sphinxext.plot_directive
    extensions += [matplotlib.sphinxext.plot_directive.__name__]

@larrybradley mentioned maybe it was

https://github.com/sphinx-doc/sphinx/blob/f6882d74663438b281a1260684a16e97696ed6d9/sphinx/ext/viewcode.py#L193

                parents.append({
                    'link': urito(pagename, '_modules/' +
                                  parent.replace('.', '/')),

which leads to

https://github.com/sphinx-doc/sphinx/blob/f6882d74663438b281a1260684a16e97696ed6d9/sphinx/util/osutil.py#L58

but hard to tell where the bug is without knowing the values of from (or base) and to strings.

bsipocz commented 4 years ago

oh, that indeed looks like to be it!

Does either of you digging into it for a fix, or shall I?

pllim commented 4 years ago

@bsipocz , I have not gotten to it yet and unlikely this week, so feel free to take it. Thanks! 🙇‍♀

bsipocz commented 4 years ago

Sounds good, this will be my rewards project this week then 😅

larrybradley commented 4 years ago

BTW, on first glance it appeared that was the bug (https://github.com/sphinx-doc/sphinx/blob/f6882d74663438b281a1260684a16e97696ed6d9/sphinx/ext/viewcode.py#L193), but I briefly tried "fixing" it and nothing change. The bug may be elsewhere.

Also, I noticed that the PNG, PDF, etc. links also have double slashes in the URL (if you look at the HTML source code). But those links work somehow.

pllim commented 4 years ago

Still should be avoided regardless. I think it becomes an issue if there is some sort of ambiguity in resolving the server-side contents.

https://stackoverflow.com/questions/20523318/is-a-url-with-in-the-path-section-valid

larrybradley commented 2 months ago

This is no longer an issue.