slint-ui / document-features

Extract documentation for the feature flags from comments in Cargo.toml
Apache License 2.0
173 stars 13 forks source link

Format features with HTML tags as `rustdoc` does #12

Closed FedericoStra closed 2 years ago

FedericoStra commented 2 years ago

Hello!

With this PR I suggest formatting the features as

<span class="stab portability"><code>feature</code></span>

instead of

**`feature`**

so that they look the same as the ones automatically generated by rustdoc.

If you don't like this as default, I can put it behind a feature such as html-style or something...

ogoffart commented 2 years ago

Thanks for the PR.

I could you elaborate what are the class stab and portability? is rustdoc using these in its .css ? What's your usecase?

FedericoStra commented 2 years ago

I could you elaborate what are the class stab and portability? is rustdoc using these in its .css ?

With the compiler unstable feature

#![feature(doc_auto_cfg)]

rustdoc automatically documents the features which are required for every item in the crate. You can see an example in this crate's documentation https://docs.rs/should-color/latest/should_color/, where the functions and the constants have a feature label next to them.

The generated HTML for these labels is for instance

<span class="stab portability" title="Available on crate feature `clap` only"><code>clap</code></span>

These HTML tags are styled by rustdoc.css, light.css and dark.css (you can find them in target/doc/ after you run cargo doc).

FedericoStra commented 2 years ago

What's your usecase?

Nothing in particular. I simply thought it could be aesthetically pleasing to display the features with a uniform style across the whole documentation.

FedericoStra commented 2 years ago

I know that currently the CI tests are failing. I'll fix them if there is interest in this PR. And as I said I can leave the default style as is and put this behind a feature (css-style, docsrs-style, rustdoc-style, ...).

ogoffart commented 2 years ago

I just wonder if there class names are stable enough. In the mean time, if you want to match something you can, i guess put the #[document-feature] macro in something like this:

//! <div class="my_class"> 
#![doc = document_features::document_features!()]
//! </div>

So you can match in your css with .my_class stong or something like that.

FedericoStra commented 2 years ago

Well...

Sure, but hunting the features in CSS would be imprecise because

.my_class strong > code {};

can match **`other unintended things`**.

Moreover, this would leave on the developer the burden of injecting this custom styling in the generated docs via e.g.

and one would also need to provide different styles for light/dark themes of the docs. It'd be the developer's responsibility to make sure that the style matches the rest of the document.

On the other hand, generating the right HTML tags allows to directly get the correct coherent styling for free, both locally and on docs.rs (notice that the two stylings are different: with the default light theme, locally the features are purple, on docs.rs they are faint yellow).

FedericoStra commented 2 years ago

I added the feature html which enables this new behavior. The tests are fixed and pass both with and without html.

ogoffart commented 2 years ago

I'm not sure a cargo feature is the right way to configure the output. And also i'm concerned that these css class are not meant to be stable and used like that.

How about this:

#![doc = document_features::document_features!(feature_template="* <span class=\"stab portability\"><code>{feature}</code></span>{default} —{doc}")]

Internally, we would just use String's replace funciton to replace {feature}, {default} and {doc}

What do you think?

FedericoStra commented 2 years ago

I'm not sure a cargo feature is the right way to configure the output. How about this:

#![doc = document_features::document_features!(feature_template="* <span class=\"stab portability\"><code>{feature}</code></span>{default} —{doc}")]

I agree that the Cargo feature is not necessarily the best way. It was just one of the possible ways to maintain backward compatibility.

We can also have the simpler

#![doc = document_features::document_features!(feature_label="<span class=\"stab portability\"><code>{feature}</code></span>")]

so that the user can customize the label without worrying about the rest of the pagination.

I've never developed a procedural macro before, so it'll take a bit of time for me to refactor it as you suggested. I guess this is the right circumstance for me to learn proc macros.

And also i'm concerned that these css class are not meant to be stable and used like that.

I'm not 100% sure they are considered stable either. They seem to have appeared around Rust 1.34 (you can see some of them in the front page of the corresponding std docs dating back to 2019-04-10). In any case, they are something that can be updated if the next releases of rustdoc change the generated HTML for these labels.

ogoffart commented 2 years ago

Your suggestion also makes sense.

I've never developed a procedural macro before, so it'll take a bit of time for me to refactor it as you suggested.

Right, the macro currently don't parse its attribute. And the fact that it doesn't depend of syn (and i'd like to keep it this way) won't make it easier. You can maybe get inspired by this funciton, for example: https://github.com/taiki-e/const_fn/blob/main/src/lib.rs#L150 (the const_fn(feature = "...") seems to be similar to what we'd like to parse)

FedericoStra commented 2 years ago

In the meantime I've rewritten the implementation by eliminating the feature html and making document_features! callable in both these ways:

#![doc = document_features::document_features!()]
#![doc = document_features::document_features!(feature_label = "<span class=\"stab portability\"><code>{feature}</code></span>")]

And the fact that it doesn't depend of syn (and i'd like to keep it this way) won't make it easier.

Unfortunately, in my first attempt at proc macros I ended up relying on syn... :confused:

You can maybe get inspired by this funciton, for example: https://github.com/taiki-e/const_fn/blob/main/src/lib.rs#L150 (the const_fn(feature = "...") seems to be similar to what we'd like to parse)

It seems very similar indeed. I'll have a look at it to learn how to avoid using syn.

ogoffart commented 2 years ago

Another thing not to foget: documentation and changelog entry.

FedericoStra commented 2 years ago

But yeah, i'd prefer not depend on syn if possible, it shouldn't be too hard to do without. Would you like to give it a try?

Done. Unfortunately, now the string argument must be either "..." or r"...", raw strings delimited by #'s such as r#"..."# are not accepted anymore (I wanted to keep the implementation simple). I have added doc-tests that check this behavior.

It's not the cleanest code for sure (the syn version was cleaner), but it does its job.

FedericoStra commented 2 years ago

Not handling raw string literals such as r#"..."# really bothers me though... I'll find a reasonably concise way of accepting them.

ogoffart commented 2 years ago

Not handling raw string literals such as r#"..."# really bothers me though... I'll find a reasonably concise way of accepting them.

You can use .trim_match("#") or something like that