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

Stabilize rustdoc theme options #54733

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@GuillaumeGomez
Member

GuillaumeGomez commented Oct 1, 2018

Fixes #54730.

It's been a loooooong time now so I guess it's time to stabilize it.

cc @QuietMisdreavus

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Oct 1, 2018

r? @steveklabnik

(rust_highfive has picked a reviewer for you, use r? to override)

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Oct 4, 2018

Have these options received any use? Why are we stabilizing them now other than "it's been a few months"?

(For context: this was initially merged back in January.)

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Oct 5, 2018

I have the use of them for some libraries of mine. That'd be nice be able to leave nightly for once. :)

@Centril Centril added the relnotes label Oct 6, 2018

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Oct 9, 2018

In that case, do these options have any tests? I'd like to make sure that, for example, an incomplete theme gets rejected by both --theme and --theme-checker, and that a complete theme gets added to the theme picker. (These may need to be run-make tests? I'm not sure how the working directory is set when resolving arguments given to compiletest. It may be necessary to figure out how to call htmldocck.py to parse the output, or to find some other means to scan the output to make sure the theme is selected.)

This also brings up the conversation we had back when this was merged, about what kind of stability guarantees we have about it. If we have a test for "good theme gets accepted and shown on the page", then that test will break every time we add new rules to the theme CSS. I guess this is to be expected, since it would already happen if you changed light.css without changing dark.css.

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Oct 9, 2018

Do you want to wait for UI tests to be merged first (and we should really try to set it up as soon as possible, too much issues are coming from this lack of test!)? We can add tests the theme-checker as well (testing the test is important too 😛). Does that seem acceptable to you?

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Oct 9, 2018

I don't think we need to wait for the UI tests to ensure the CLI flags work as necessary - the tests it can add are orthogonal to the ones i suggested. I feel like the only thing it can add is to make sure that no unexpected changes happen to the HTML that invalidate a CSS rule, but that was already proposed on the UI tests without having to bring custom themes into it.

@QuietMisdreavus QuietMisdreavus self-assigned this Oct 17, 2018

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Oct 17, 2018

cc @rust-lang/rustdoc for consensus

In addition to the tests that are still outstanding, does this flag have any docs in the Rustdoc Book? We should make sure that stable items have documentation.

@bors

This comment has been minimized.

Contributor

bors commented Nov 5, 2018

☔️ The latest upstream changes (presumably #55515) made this pull request unmergeable. Please resolve the merge conflicts.

@Mark-Simulacrum

This comment has been minimized.

Member

Mark-Simulacrum commented Nov 15, 2018

@QuietMisdreavus @rust-lang/rustdoc Can we start an FCP to merge here perhaps? I've marked this as waiting on team for the time being.

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Nov 15, 2018

@Mark-Simulacrum This still needs tests and needs to move the docs to the stable page (and now has a merge conflict because it was written before the CLI centralization), but we can at least ping everyone...

@rust-lang/rustdoc This PR proposes to stabilize the --themes and --theme-checker flags, which allow users to add their own themes to a specific set of docs. These were introduced at the same time as the initial dark theme, and have a safeguard to ensure that a theme is covering the same elements as the default "light" theme.

One thing that is not being stabilized, and likely will not ever be stabilized, is the CSS rules themselves, or the semantic tag layout of the docs. This means that themes are able to "break" between versions, as we add or remove things from the default themes. Historically we have never made this promise (and specifically declared the layout perma-unstable in the initial --themes discussion and the discussion around --extend-css), but the fact that it can cause the same doc command with a certain custom theme to break between versions is something to note. (In fact this is partly why i'm hesitant to stabilize this - i'm not aware of anything else that follows this kind of rule.)

@rfcbot fcp merge

@rfcbot concern stability of css rules

@rfcbot

This comment has been minimized.

rfcbot commented Nov 15, 2018

Team member @QuietMisdreavus has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:stabilize-rustdoc-theme branch from 1740393 to 478d7e4 Nov 24, 2018

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 24, 2018

@frewsxcv

This comment has been minimized.

Member

frewsxcv commented Nov 24, 2018

By “stabilizing” theme options, does that restrict us from changing the style and layout of the rustdoc output in the future?

@GuillaumeGomez Also, what do you hope to achieve by stabilizing it other than you want to use the stable compiler?

@frewsxcv

This comment has been minimized.

Member

frewsxcv commented Nov 24, 2018

One thing that is not being stabilized, and likely will not ever be stabilized, is the CSS rules themselves, or the semantic tag layout of the docs.

Ah, missed @QuietMisdreavus’s comment above

After this flag stabilizes, if someone writes their own theme and the rustdoc team changes the layout between releases, their theme is going to silently break for them when they upgrade. Since they were using a stable feature flag of official rust-lang tooling, I'm worried this will cause frustration and distrust between the users and us.

@steveklabnik

This comment has been minimized.

Member

steveklabnik commented Nov 24, 2018

Yeah, I'm not sure myself. I can see it both ways. hmm

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 25, 2018

@frewsxcv Just like you said: it's not stabilizing the UI but the UI generator. :)

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Nov 30, 2018

I think @frewsxcv said it best. The more i think about it, the less certain i am about allowing it on stable like this. It doesn't feel useful enough to allow that potential "stable breakage". Any theme a user adds with this option will only be available on the docs that are generated on that run, meaning it's only generally useful if they're generating all their docs locally (and are using a theme that fits their display/vision/etc) or if they're publicly hosting docs for a crate, losing the people who default to using docs.rs to look up crate docs. If docs.rs wants to add a custom theme, the fact that this is unstable doesn't matter, because it's already using nightly-only items (and is official code anyway).

@GuillaumeGomez I'm curious, does the current theme-checker code have a way to make sure the light theme isn't missing rules that the dark theme is adding? As far as i know, the default theme-checker only tests the other way around.

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Dec 1, 2018

It's a bit subtle (and a bit more simple): each theme is required to implement the same rules. If the dark theme adds new rules that aren't in the light one, then it'll break (since they're not identical anymore).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment