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

Documentation Issue on Read the Docs #2300

Closed
critfest opened this issue Jul 31, 2019 · 9 comments
Closed

Documentation Issue on Read the Docs #2300

critfest opened this issue Jul 31, 2019 · 9 comments

Comments

@critfest
Copy link

@critfest critfest commented Jul 31, 2019

Environment info

Operating System:

CPU/GPU model:

C++/Python/R version:

LightGBM version or commit hash:

Error message

Reproducible examples

Steps to reproduce

  1. Go to https://lightgbm.readthedocs.io/en/latest/Installation-Guide.html
  2. Click any link with a .rst extension (there were at least four I found)
  3. You get an error page stating:
    " SORRY /
    \ /
    \ This page does /
    ] not exist yet. [ ,'|
    " like this: https://lightgbm.readthedocs.io/en/latest/README.rst
@StrikerRUS

This comment has been minimized.

Copy link
Collaborator

@StrikerRUS StrikerRUS commented Jul 31, 2019

@critfest
Sorry, I can't reproduce the issue. Please point to the concrete link.

@hayesall

This comment has been minimized.

Copy link
Contributor

@hayesall hayesall commented Jul 31, 2019

@critfest I've only been able to reproduce this behavior by clicking links in view-source mode.

<a class="reference external" href="./FAQ.rst#i-am-using-windows-should-i-use-visual-studio-or-mingw-for-compiling-lightgbm">Question 4</a>

Can you include the browser you are using (and version if available)? I haven't been able to reproduce this in Firefox 68.0.1 or Chrome 75.

@jameslamb

This comment has been minimized.

Copy link
Collaborator

@jameslamb jameslamb commented Jul 31, 2019

We should be replacing all of those per #2272 (comment)

@jameslamb jameslamb added the bug label Jul 31, 2019
@StrikerRUS

This comment has been minimized.

Copy link
Collaborator

@StrikerRUS StrikerRUS commented Jul 31, 2019

We should be replacing all of those per #2272 (comment)

Yeah, RTD requires JS enabled. @critfest Please ensure that JS is enabled in your browser or any third-party software is not blocking it.

@jameslamb

This comment has been minimized.

Copy link
Collaborator

@jameslamb jameslamb commented Jul 31, 2019

@StrikerRUS OH! When you showed me that before, I didn't realize it ran on page load in the browser.

I think we should write a hook in our conf.py that does that replacement when generating the HTML content. That way, we'd never have a browser-specific problem (it would just be static HTML) but it would still preserve the desired behavior of having .rst links so links inside the repo work.

I would be happy to write this if you think it's a good idea @StrikerRUS (and would love your input @hayesall )!

@StrikerRUS

This comment has been minimized.

Copy link
Collaborator

@StrikerRUS StrikerRUS commented Jul 31, 2019

@jameslamb I considered this option when was initially reworking docs. I abandoned the idea to replace links at Python side due to additional dependencies (e.g. bs4). The current links replacement is done by very simple jQuery function (jQuery is already imbedded into RTD). I'm not sure that it's possible to do the same at Python side elegantly without additional dependencies. Nowadays, practically all sites cannot work without JS properly, and the only requirement we have is enabled JS.

That way, we'd never have a browser-specific problem (it would just be static HTML)

We have other things at JS side which are impossible to replace with Python equivalent. Moreover, RTD itself cannot work without JS.

@jameslamb

This comment has been minimized.

Copy link
Collaborator

@jameslamb jameslamb commented Jul 31, 2019

Ok makes sense! Thanks for educating me on some of the history of our docs this week. The "I don't want to take on a BeautifulSoup dependency" I think could be avoided (I was thinking of literally matching a regular expression for .rst links) but the argument that we have other JS stuff which is harder to replace is convincing.

@StrikerRUS StrikerRUS removed the bug label Aug 1, 2019
@StrikerRUS StrikerRUS closed this Aug 1, 2019
@hayesall

This comment has been minimized.

Copy link
Contributor

@hayesall hayesall commented Aug 3, 2019

@StrikerRUS Hey, sorry for not following up on this earlier, but I think it would be worth adding this to #2302 as "noscript-compatible documentation."

People sometimes block JavaScript for security or privacy reasons. Most of this documentation is generated as static content, so retaining an option for users who disable JavaScript should be considered.

@StrikerRUS

This comment has been minimized.

Copy link
Collaborator

@StrikerRUS StrikerRUS commented Aug 3, 2019

@hayesall

Hey, sorry for not following up on this earlier, but I think it would be worth adding this to #2302 as "noscript-compatible documentation."

Unfortunately, it's unresolvable issue, which we'll never fix. I don't think that we need such issue in our list.

Moreover, RTD site itself cannot work without JS enabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.