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

Add --extend-css option to rustdoc #32230

Merged
merged 4 commits into from Apr 7, 2016

Conversation

@GuillaumeGomez
Copy link
Member

commented Mar 13, 2016

Fixes #32223

r? @brson

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 13, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2016

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

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 14, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2016

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

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 15, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2016

@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

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2016

@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

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2016

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

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:extend_css branch from f780790 to f08e4b4 Mar 15, 2016

@@ -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"),

This comment has been minimized.

Copy link
@brson

brson Mar 16, 2016

Contributor

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).

// we now check if unstable options are allowed and if we're in a nightly build
let has_z_unstable_options = matches.opt_strs("Z")
.iter()
.any(|x| *x == "unstable-options");

This comment has been minimized.

Copy link
@brson

brson Mar 16, 2016

Contributor

It looks like this will accept anything after -Z. I think there needs to be a branch returning an error if any z-flags are passed besides unstable_options.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:extend_css branch from f08e4b4 to 70d444b Mar 16, 2016

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 {

This comment has been minimized.

Copy link
@mitaa

mitaa Mar 16, 2016

Contributor

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.

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Mar 16, 2016

Author Member

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.

// we now check if unstable options are allowed and if we're in a nightly build
let mut has_z_unstable_options = false;
for flag in matches.opt_strs("Z").iter() {
if *flag != "unstable-options" {

This comment has been minimized.

Copy link
@mitaa

mitaa Mar 16, 2016

Contributor

nit: These if conditions use !n and n - could the if and else if be swapped, changing the if flag != "unstable-options" to just else?

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Mar 16, 2016

Author Member

I'm not sure about what @brson wants in here. He said some other options than "unstable-options" could be added so I wrote my code in this direction. But for now I'll just do it the simplest way.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:extend_css branch from 70d444b to 52f791e Mar 16, 2016

@brson

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2016

@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.

@brson brson added the T-tools label Mar 16, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 16, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Mar 16, 2016

@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

This comment has been minimized.

Copy link
Member

commented Mar 16, 2016

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

This comment has been minimized.

Copy link
Member

commented Mar 17, 2016

@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

This comment has been minimized.

Copy link
Member Author

commented Mar 17, 2016

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

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 17, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Mar 17, 2016

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

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 18, 2016

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

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2016

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

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2016

Any news?

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2016

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

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:extend_css branch from d87e307 to 9fe7092 Mar 24, 2016

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:extend_css branch from 85f5449 to dff79e4 Apr 4, 2016

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2016

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

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Apr 5, 2016

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2016

🔒 Merge conflict

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:extend_css branch from dff79e4 to 3665ba0 Apr 5, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Apr 5, 2016

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2016

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

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

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2016

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

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:extend_css branch from 3665ba0 to d0a91a3 Apr 6, 2016

@@ -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>,

This comment has been minimized.

Copy link
@mitaa

mitaa Apr 6, 2016

Contributor

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

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Apr 6, 2016

Author Member

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".

This comment has been minimized.

Copy link
@mitaa

mitaa Apr 6, 2016

Contributor

Can't you do cx.shared.css_file_extension?

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Apr 6, 2016

Author Member

Oh, indeed. Modified too quickly.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:extend_css branch from d0a91a3 to 669edfa Apr 6, 2016

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2016

Re-updated to last rustdoc version.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Apr 6, 2016

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2016

⌛️ Testing commit 669edfa with merge a828124...

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

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2016

💔 Test failed - auto-mac-32-opt

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2016

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

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Apr 6, 2016

@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

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2016

⌛️ Testing commit 669edfa with merge a9f34c8...

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

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

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@GuillaumeGomez GuillaumeGomez deleted the GuillaumeGomez:extend_css branch Apr 28, 2016

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