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

Caution against dynamic prettyurls #2402

Merged
merged 5 commits into from
Jan 26, 2024
Merged

Caution against dynamic prettyurls #2402

merged 5 commits into from
Jan 26, 2024

Conversation

goerz
Copy link
Member

@goerz goerz commented Jan 15, 2024

We should steer people away from using

prettyurls = get(ENV, "CI", nothing) == "true"

(or even prettyurls = false) and towards using LiveServer.

It is best to have a consistent setting and to always use a webserver when viewing documentation locally. This avoids situations where there might be subtle difference with prettyurls=true or prettyurls=false and the documentation might work correctly when viewed locally, but be subtly broken (without an error message!) when deploying on GitHub Action. One such situation is a @raw block that references a local image. The correct relative path of that image would be different depending on the prettyurls setting.

Closes #2395

@goerz
Copy link
Member Author

goerz commented Jan 15, 2024

Just a proposal to get things started. I have no particular attachment to any of the text. Feel free to tell me to change something, or directly push to the branch with whatever text you like.

We should steer people away from using

    prettyurls = get(ENV, "CI", nothing) == "true"

(or even `prettyurls = false`) and towards using `LiveServer`.

It is best to have a consistent setting and to always use a webserver
when viewing documentation locally. This avoids situations where there
might be subtle difference with `prettyurls=true` or `prettyurls=false`
and the documentation might work correctly when viewed locally, but be
subtly broken (without an error message!) when deploying on GitHub
Action. One such situation is a `@raw` block that references a local
image. The correct relative path of that image would be different
depending on the `prettyurls` setting.
@goerz goerz force-pushed the mg/prettyurls-docs branch from face7ff to f8bd6ba Compare January 15, 2024 18:08
@mortenpi
Copy link
Member

I'm actually kinda leaning towards just removing the

get(ENV, "CI", nothing) == "true"

suggestion. Or maybe putting it in a !!! warning admonition, saying that "you may run into this, but it's not really recommended".

@goerz goerz force-pushed the mg/prettyurls-docs branch from 34cc080 to 636089d Compare January 16, 2024 03:58
@goerz goerz force-pushed the mg/prettyurls-docs branch from 636089d to 8d0dec7 Compare January 16, 2024 04:00
@goerz
Copy link
Member Author

goerz commented Jan 16, 2024

docs/src/man/guide.md Outdated Show resolved Hide resolved
Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @goerz!

@mortenpi mortenpi enabled auto-merge (squash) January 26, 2024 04:00
@mortenpi mortenpi merged commit deecbd7 into master Jan 26, 2024
19 of 22 checks passed
@mortenpi mortenpi deleted the mg/prettyurls-docs branch January 26, 2024 04:19
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.

Remove CI-dependent prettyurls and recommend LiveServer.jl?
2 participants