Add --extend-css option to rustdoc #32230

GuillaumeGomez · 2016-03-13T14:37:09Z

alexcrichton · 2016-03-13T22:27:16Z

I would prefer we not land any new insta-stable options into rustdoc. We've already got infrastructure in the compiler for truly unstable options, and rustdoc should likely adopt a similar strategy.

GuillaumeGomez · 2016-03-13T23:49:00Z

"insta-stable"? "truly unstable"? Sorry but I don't understand these two points. Could you explain them please? (Or give me a link?)

alexcrichton · 2016-03-14T16:44:37Z

Yes, to clarify I mean:

This new flag is instantly stable, we don't get a period of instability where we can make changes.
The compiler now has support for unstable flags, meaning those only available on nightly and requiring an opt-in to use.

GuillaumeGomez · 2016-03-14T21:16:59Z

@alexcrichton: I updated the code. Does this check seems enough for you?

alexcrichton · 2016-03-15T21:26:44Z

No, the compiler has the ability to specify options with a stability level attached to them. The options are always parsed but any usage of an unstable option is gated. That's functionally what happens here but it's very brittle as it'd be tied to each option specifically.

All of this is also with a grain of salt as well, as we still have yet to make a decision at all as to whether to allow custom css like this. I thought that in previous PRs we decided not to?

GuillaumeGomez · 2016-03-15T21:33:49Z

@alexcrichton: In the PR you're referring to, I tried to propose alternative style by providing css and to allow readers to switch between themes while reading the docs. This one is very different since it targets docs builders, and allow them to customize the css. I exposed the idea to @brson and he said it was ok (at least to open the PR).

The thing I understood rust people didn't want was to change the official doc style. This one doesn't change it.

PS: if this PR goes further, where can I find a "gated" option, so I can do the same for this one?

brson · 2016-03-15T21:50:10Z

@GuillaumeGomez it sounds like we need to add feature gating to rustdoc.

Here's basically what we need to do:

Add a -Z unstable-features flag to rustdoc.
When rustdoc sees the --extra-css flag, it checks whether -Z unstable-features was also passed. If not it exits with this error.
If -Z unstable-features itself is passed then it checks whether get_unstable_features_setting allows unstable features. If not then it exits with this error.

Here's the equivalent code that deals with command-line feature gating in rustc.

This is similar to the code you've previously added to make compile-fail work with rustdoc, but applied to its command-line parameters.

Sorry for the inconvenience.

GuillaumeGomez · 2016-03-15T22:30:10Z

Oh I see, I thought a full code suite was made for it. Thanks @brson!

brson · 2016-03-16T00:39:50Z

src/librustdoc/lib.rs

@@ -187,6 +189,7 @@ pub fn opts() -> Vec<getopts::OptGroup> {
        optopt("e", "extend-css",
               "to redefine some css rules with a given file to generate doc with your \
                own theme", "PATH"),
+        optmulti("Z", "", "enable unstable options (only on nightly build)", "FLAG"),


Could the text here be "internal and debugging options" to more closely match rustc? -Z isn't just for the unstable-options flag (in theory).

mitaa · 2016-03-16T17:48:27Z

src/librustdoc/lib.rs

+fn is_unstable_flag_enabled(nightly_build: bool, has_z_unstable_options: bool,
+                                  flag_name: &str) {
+    // check if unstable for --extend-css option
+    match if !nightly_build {


nit:

maybe this could just be a let instead of match and early return in the else case?

could this function be named check_*? With is_* I'd expect a bool return value.

maybe this could just be a let instead of match and early return in the else case?

Good idea.

could this function be named check__? With is__ I'd expect a bool return value.

At first, it was supposed to. I'll rename it.

brson · 2016-03-16T20:21:18Z

@GuillaumeGomez I'm going to have to discuss this with the tools team since I'm getting significant pushback about adding the feature to rustdoc. I'll have to get back to you, but Ill try to it soon.

alexcrichton · 2016-03-16T20:28:14Z

From a technical point of view I think that this also needs some refactoring. The compiler has support for directly attaching a stability level to a command line option, and this is just an ad-hoc check whenever a command line option is consumed. Additionally, this is essentially duplicating the infrastructure in the compiler for this feature, and ideally we would not duplicate any of it.

GuillaumeGomez · 2016-03-16T21:10:04Z

@alexcrichton: This is frustrating to stop new things to emerge because of such missing mechanisms. However I understand so I can only agree to your opinion. Would you prefer that I create (or extend opts?) a common code for this?

nrc · 2016-03-16T21:22:03Z

Do we have a stability policy for tools like rustdoc? I did not think that we offered stability guarantees of any kind for rustdoc. I guess maybe it is wrapped up in the fact that rustdoc is in the main repo, not the nursery or something? I don't think we've ever audited the existing options, maybe we should (quite likely I just missed it if we have though).

Anyway, I'm strongly in favour of this PR. I guess it should be unstable, but I don't feel like stability for rustdoc is super-important (c.f., the compiler).

alexcrichton · 2016-03-17T21:40:37Z

@GuillaumeGomez

This is frustrating to stop new things to emerge because of such missing mechanisms.

It's also frustrating to have code duplication within the project which causes subtle bugs to emerge in the future because they're not behaving the same way or because one was fixed without the other.

I would recommend refactoring whatever's in rustc so it's usable between both the compiler and rustdoc.

@nrc

Do we have a stability policy for tools like rustdoc?

Perhaps not written down, but do we have one written down for rustc? I'm under the assumption that it's basically almost all entirely stable today. We did a brief audit of compiler flags before 1.0, but we just didn't really have the effort or time to do it for rustdoc as well.

GuillaumeGomez · 2016-03-17T21:42:47Z

@alexcrichton: Before doing anything, I prefer wait the end of the debate so I don't work for nothing.

alexcrichton · 2016-03-17T22:04:30Z

There seems to be enough support for this feature that we probably want to land it in some capacity, so if the unstable gating works robustly as the compiler does then this is likely ready to r+

GuillaumeGomez · 2016-03-17T22:06:02Z

@alexcrichton: Do you want me to directly centralize this "feature" on this PR or do you prefer I open another one?

alexcrichton · 2016-03-18T22:09:03Z

Let's do it as part of this PR as it'll just otherwise conflict

GuillaumeGomez · 2016-03-20T01:18:47Z

@alexcrichton: Done. Is it what you had in mind?

GuillaumeGomez · 2016-03-22T15:07:42Z

Any news?

bors · 2016-03-23T19:39:58Z

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

GuillaumeGomez · 2016-04-04T21:23:39Z

So the code is now the same as the previous one, just moved from another place.

alexcrichton · 2016-04-05T17:24:11Z

@bors: r+ dff79e4e3a1e975011f1181e175b4e902c481809

bors · 2016-04-05T21:19:25Z

🔒 Merge conflict

alexcrichton · 2016-04-05T22:21:29Z

@bors: r+ 3665ba08cf38585466b57f696efc434024b8b3f4

bors · 2016-04-06T00:03:55Z

⌛ Testing commit 3665ba0 with merge 541a166...

bors · 2016-04-06T00:22:18Z

💔 Test failed - auto-mac-64-opt-rustbuild

mitaa · 2016-04-06T08:57:00Z

src/librustdoc/html/render.rs

@@ -99,6 +99,9 @@ pub struct Context {
    /// publicly reused items to redirect to the right location.
    pub render_redirect_pages: bool,
    pub shared: Arc<SharedContext>,
+    /// The given user css file which allow to customize the generated
+    /// documentation theme.
+    pub css_file_extension: Option<PathBuf>,


I think this is redundant now that this is also in SharedContext?

The issue comes from the fact that this option must be used in both Context and SharedContext. I tried to have it only in SharedContext but the code started to change way too much compared to the "gain".

Can't you do cx.shared.css_file_extension?

Oh, indeed. Modified too quickly.

GuillaumeGomez · 2016-04-06T10:42:44Z

Re-updated to last rustdoc version.

alexcrichton · 2016-04-06T16:37:21Z

@bors: r+ 669edfa

bors · 2016-04-06T18:15:25Z

⌛ Testing commit 669edfa with merge a828124...

bors · 2016-04-06T20:26:19Z

💔 Test failed - auto-mac-32-opt

GuillaumeGomez · 2016-04-06T20:32:17Z

Why is there a line longer than 100 chars in librustc/traits/error_reporting.rs?

alexcrichton · 2016-04-06T20:45:55Z

@bors: retry

On Wed, Apr 6, 2016 at 1:32 PM, Guillaume Gomez notifications@github.com
wrote:

Why is there a line longer than 100 chars in librustc/traits/
error_reporting.rs?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#32230 (comment)

bors · 2016-04-07T00:11:45Z

⌛ Testing commit 669edfa with merge a9f34c8...

@brson

Add --extend-css option to rustdoc Fixes #32223 r? @brson

bors · 2016-04-07T03:11:12Z

alexcrichton assigned brson Mar 14, 2016

GuillaumeGomez force-pushed the extend_css branch from f780790 to f08e4b4 Compare March 15, 2016 23:51

brson reviewed Mar 16, 2016
View reviewed changes

GuillaumeGomez force-pushed the extend_css branch from f08e4b4 to 70d444b Compare March 16, 2016 14:41

mitaa reviewed Mar 16, 2016
View reviewed changes

GuillaumeGomez force-pushed the extend_css branch from 70d444b to 52f791e Compare March 16, 2016 18:11

brson added the T-tools label Mar 16, 2016

GuillaumeGomez force-pushed the extend_css branch from d87e307 to 9fe7092 Compare March 24, 2016 00:09

GuillaumeGomez force-pushed the extend_css branch from 85f5449 to dff79e4 Compare April 4, 2016 21:03

GuillaumeGomez added 3 commits April 5, 2016 01:39

Add --extend-css option to rustdoc

ab835a1

Add nightly check on rustdoc --extend-css option

f34ae3f

Centralize nightly compiler flags handling

ded701b

GuillaumeGomez force-pushed the extend_css branch from dff79e4 to 3665ba0 Compare April 5, 2016 21:52

GuillaumeGomez force-pushed the extend_css branch from 3665ba0 to d0a91a3 Compare April 6, 2016 08:24

mitaa reviewed Apr 6, 2016
View reviewed changes

Update to last rustdoc version

669edfa

GuillaumeGomez force-pushed the extend_css branch from d0a91a3 to 669edfa Compare April 6, 2016 10:42

bors added a commit that referenced this pull request Apr 7, 2016

Auto merge of #32230 - GuillaumeGomez:extend_css, r=alexcrichton

a9f34c8

Add --extend-css option to rustdoc Fixes #32223 r? @brson

bors merged commit 669edfa into rust-lang:master Apr 7, 2016

mitaa mentioned this pull request Apr 7, 2016

rustdoc: Remove the json-{input, output} format #32773

Merged

GuillaumeGomez deleted the extend_css branch April 28, 2016 13:42

GuillaumeGomez mentioned this pull request May 8, 2016

Add "extension-css" option rust-lang/cargo#2664

Closed

Add --extend-css option to rustdoc #32230

Add --extend-css option to rustdoc #32230

Conversation

GuillaumeGomez commented Mar 13, 2016

alexcrichton commented Mar 13, 2016

GuillaumeGomez commented Mar 13, 2016

alexcrichton commented Mar 14, 2016

GuillaumeGomez commented Mar 14, 2016

alexcrichton commented Mar 15, 2016

GuillaumeGomez commented Mar 15, 2016

brson commented Mar 15, 2016

GuillaumeGomez commented Mar 15, 2016

brson Mar 16, 2016

Choose a reason for hiding this comment

mitaa Mar 16, 2016

Choose a reason for hiding this comment

GuillaumeGomez Mar 16, 2016

Choose a reason for hiding this comment

brson commented Mar 16, 2016

alexcrichton commented Mar 16, 2016

GuillaumeGomez commented Mar 16, 2016

nrc commented Mar 16, 2016

alexcrichton commented Mar 17, 2016

GuillaumeGomez commented Mar 17, 2016

alexcrichton commented Mar 17, 2016

GuillaumeGomez commented Mar 17, 2016

alexcrichton commented Mar 18, 2016

GuillaumeGomez commented Mar 20, 2016

GuillaumeGomez commented Mar 22, 2016

bors commented Mar 23, 2016

GuillaumeGomez commented Apr 4, 2016

alexcrichton commented Apr 5, 2016

bors commented Apr 5, 2016

alexcrichton commented Apr 5, 2016

bors commented Apr 6, 2016

bors commented Apr 6, 2016

mitaa Apr 6, 2016

Choose a reason for hiding this comment

GuillaumeGomez Apr 6, 2016

Choose a reason for hiding this comment

mitaa Apr 6, 2016

Choose a reason for hiding this comment

GuillaumeGomez Apr 6, 2016

Choose a reason for hiding this comment

GuillaumeGomez commented Apr 6, 2016

alexcrichton commented Apr 6, 2016

bors commented Apr 6, 2016

bors commented Apr 6, 2016

GuillaumeGomez commented Apr 6, 2016

alexcrichton commented Apr 6, 2016

bors commented Apr 7, 2016

bors commented Apr 7, 2016