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

rustdoc: print non-self arguments of bare functions and struct methods on their own line #36679

Merged
merged 6 commits into from
Oct 12, 2016

Conversation

QuietMisdreavus
Copy link
Member

This change alters the formatting rustdoc uses when it creates function and struct method documentation. For bare functions, each argument is printed on its own line. For struct methods, non-self arguments are printed on their own line. In both cases, no line breaks are introduced if there are no arguments, and for struct methods, no line breaks are introduced if there is only a single self argument. This should aid readability of long function signatures and allow for greater comprehension of these functions.

I've run rustdoc with these changes on my crate egg-mode and its set of dependencies and put the result on my server. Of note, here are a few shortcut links that highlight the changes:

This PR consists of two commits: one for bare functions and one for struct methods.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @steveklabnik (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@steveklabnik
Copy link
Member

/cc @rust-lang/docs @rust-lang/tools

2016-09-23-151129_322x223_scrot

this feels really excessive. I do agree that sometimes signatures can be rough, though. Hm.

@QuietMisdreavus
Copy link
Member Author

Yeah, with the shorter signatures the added newlines can be a net negative as far as noise is concerned. On the other hand, I'd prefer it to be consistent with longer signatures, rather than saying "minimum one non-self argument before adding linebreaks" or something like that.

@durka
Copy link
Contributor

durka commented Sep 23, 2016

The arguments should at least be aligned with the opening parenthesis if they have to go on the new line. It seems like a length threshold would be good.

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Sep 23, 2016

This is the second PR about code formatting in rustdoc formatting in an hour. Plus there's #36654 which isn't even a day old. Where's all the rustdoc code formatting buzz coming from all of a sudden? 😄 Don't get me wrong, I'm not complaining.

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Sep 23, 2016

As for the specific change here, I agree that it's excessive to do on virtually all signatures. Perhaps running rustdoc (like #36654 proposes for type definitions) on the signatures will be better? It would keep short signatures (regardless of the exact number of parameters) on one line but still split bigger signatures.

@QuietMisdreavus
Copy link
Member Author

@durka I like that suggestion for the indentation. It'll be weird getting the indent level passed in due to how it's being printed, but I think it's doable.

As for a length threshold, I'll echo my earlier comment about consistency. Adding newlines based on the final length of the argument string feels like a weird thing to base the decision on, especially if short type parameters get involved. It feels like it would be worse to have three arguments fit in one line simply because they have a short binding name and type name; it would clutter up the line the same way longer signatures would now.

@hanna-kruppe
Copy link
Contributor

@QuietMisdreavus But thresholding based on the length is already done in code! Both rustfmt and most people's personal style will break signatures based on length, not on argument count.

@QuietMisdreavus
Copy link
Member Author

@rkruppe Except this is documentation, not code. It's not read the same way, and it doesn't need to be formatted the same way. IMO it's okay to have extra vertical space in docs because it makes the interface of the library more explicit, and aids first-time reading comprehension to see all the arguments laid out. In the actual code, you can format it differently because by the time you look at that version you're investigating how it works or already have that understanding.

I ran rustfmt (via the playground) on a sample of code from egg-mode (and a handful of other function definitions outside this gist) because I personally hadn't seen its output on these kinds of cases before. It seems to agree with the "one argument per line" rule, but only once the signature passes some length threshold. That's easy enough to set up, provided I can get the length of the leading name into the Display impl.

I surveyed a handful of other languages' standard documentations to see what styles were in use. It's hard to get a real apples-to-apples comparison, but at least I can get some concrete examples to reference for good or ill.

  • Languages that don't assign static types to function arguments seem to put all arguments on one line, since there's not a lot of extra noise (Ruby, Python, Node.js)
  • Languages that do assign static types to function arguments often put arguments on their own line (.NET, Java, C++ (MSDN)). Of note, I'm fairly sure the .NET documentation is also automatically-generated.
  • Some C++ references, however, will arbitrarily group arguments together, either by hand-written grouping or by writing adjacent arguments together so that no line breaks a length threshold once it takes the name/return type (or equivalent indentation) into account (C++ (cppreference.com), C++ (cplusplus.com))

I think it's the last approach that I'm against; arguments might have a logical grouping that would get broken up by the "fit as many on a line as you can" style. At the level that rustdoc is working, I see no way to copy any potentially-existing manual grouping from the original source code, either. I'd be okay with a sort of "all or nothing" style, where signatures can fit on one line under some threshold (which would be calculated by... byte count? .chars().count()? The same question comes in when calculating indentation, too. Since rustc doesn't allow non-ascii identifiers this question is fairly simple for the moment, but if that were allowed in the future this would need to be revisited), or have arguments one-per-line, starting on the same line as the function name. I can change the formatting of the close-paren and return type to match the style used by rustfmt; that should be no problem.

@GuillaumeGomez
Copy link
Member

This is a good idea but its exploitation isn't good from my point of view. I don't like backline forcing when it's unneeded. Also:

@rkruppe Except this is documentation, not code. It's not read the same way, and it doesn't need to be formatted the same way.

This is right and wrong at the same time. If its format is close to the code, people will read it more quickly. However, since it's doc, we can improve it more than the code could allow it.

@bluss
Copy link
Member

bluss commented Sep 23, 2016

@rkruppe Why now? Because we have docs.rs now. So instead of applying my own css tweaks and host docs myself I have to try to push upstream.

@QuietMisdreavus
Copy link
Member Author

@rkruppe @bluss For me, the timing is purely coincidence. I mused on IRC about wanting to have this change, then I researched how to make it happen. I figure, even if this is ultimately rejected and I wind up keeping my own rustdoc fork for my personally-hosted docs, the process of trying to get it accepted upstream will most likely make it better than my initial naive implementation.

@QuietMisdreavus
Copy link
Member Author

@GuillaumeGomez I guess what I'm trying to distinguish is that reading code to understand how it works and how to change it is different from reading code (and accompanying text) to understand how to use it from an outside/black-box perspective. What works in the compiled code to allow more seamless access to neighboring methods or to group related arguments together may not work as well in the separate documentation when I'm trying to figure out what parameter goes where and which one I'm missing. I cut my teeth in .NET where I had ample documentation and didn't need to see the inner workings as much. (It's also the documentation-code-block indentation style I was aping in the provided initial samples.) I'll recognize that that may not work as well here, but it's at least the direction I'm coming from.

@GuillaumeGomez
Copy link
Member

@QuietMisdreavus: Like I said, I like your idea, but not when a function has only one parameter. Maybe we should think about it when we have more than one parameter? Hard to say.

@nrc
Copy link
Member

nrc commented Sep 25, 2016

My feeling is that if we do this, rustdoc should follow rustfmt (and thus the official formatting guidelines, as they are decided) strictly. Preferably by actually calling Rustfmt (in process, it has API that should be able to handle this). It seems very bad to use different formatting here vs in Rust code.

In particular, we should probably only do this for 'long' signatures, I think for signatures that are short, this is not an improvement (but it is for longer sigs).

@QuietMisdreavus
Copy link
Member Author

Calling rustfmt directly would be difficult, since the same process that prints this information also writes the HTML anchor tags inline. I'm in the middle of wrangling the indentation rules so that it will match rustfmt's guidelines.

Just so that it can get said (or pointed towards) directly, what should we qualify as a "long" signature? What character length (if that's our standard) is "long" here?

@nrc
Copy link
Member

nrc commented Sep 25, 2016

Just so that it can get said (or pointed towards) directly, what should we qualify as a "long" signature?

Rustfmt breaks at the max line length, which is currently 100, but there is some debate about lowering it to 80 (and this is used in much of std, but not the compiler). Given the larger font here, I would use at most 80, and maybe less, depending on where we start wrapping on a typical display.

QuietMisdreavus added 3 commits September 26, 2016 16:02
* ignore signatures 80 characters or shorter
* otherwise, put arguments and return arrow on their own line, indented
  to the opening parenthesis

in doing this, most of a plain-text output has been written for rustdoc,
accessible through "alternate" Display printing: "{:#}"
@QuietMisdreavus
Copy link
Member Author

I've made some updates, to make the function formatting more like rustfmt. The rules are implemented as follows:

  • If a function's signature is 80 characters or less, it stays on one line.
  • If it's longer, parameters and the return type arrow are put on their own line, starting with the second parameter (or return arrow if there's one or zero parameters).

Implementing this amounted to creating a plain-text output for functions and struct methods, so that I wouldn't have to parse out the link tags and html entities. The code could use some style pointers.

The examples linked above should look much nicer now. There are still weird edge cases with awkward signatures, but it should be nicer than my earlier appropriation of .NET documentation style.

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Sep 27, 2016

I appreciate this a bit more. I wait to see how it's going to evolve to give my approval but I think it's on the good way!

@bjorn3
Copy link
Member

bjorn3 commented Sep 28, 2016

Please do the breaking in the browser, so if someone views the docs on a phone the arguments will be on new lines earlier than when shown on a pc.

@steveklabnik
Copy link
Member

@nrc what do you think of this now?

@nrc
Copy link
Member

nrc commented Oct 6, 2016

LGTM. Long-term I'd love to find a way to use Rustfmt directly, but this definitely looks like an improvement.

@steveklabnik
Copy link
Member

Let's :shipit:

Thank you for the patience, @QuietMisdreavus .

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Oct 10, 2016

📌 Commit 0c2b258 has been approved by steveklabnik

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Oct 11, 2016
…, r=steveklabnik

rustdoc: print non-self arguments of bare functions and struct methods on their own line

This change alters the formatting rustdoc uses when it creates function and struct method documentation. For bare functions, each argument is printed on its own line. For struct methods, non-self arguments are printed on their own line. In both cases, no line breaks are introduced if there are no arguments, and for struct methods, no line breaks are introduced if there is only a single self argument. This should aid readability of long function signatures and allow for greater comprehension of these functions.

I've run rustdoc with these changes on my crate egg-mode and its set of dependencies and put the result [on my server](https://shiva.icesoldier.me/doc-custom/egg_mode/). Of note, here are a few shortcut links that highlight the changes:

* [Bare function with a long signature](https://shiva.icesoldier.me/doc-custom/egg_mode/place/fn.reverse_geocode.html)
* [Struct methods, with single self argument and with self and non-self arguments](https://shiva.icesoldier.me/doc-custom/egg_mode/tweet/struct.Timeline.html#method.reset)
* [Bare functions with no arguments](https://shiva.icesoldier.me/doc-custom/rand/fn.thread_rng.html) and [struct methods with no arguments](https://shiva.icesoldier.me/doc-custom/hyper/client/struct.Client.html#method.new) are left unchanged.

This PR consists of two commits: one for bare functions and one for struct methods.
bors added a commit that referenced this pull request Oct 11, 2016
Rollup of 9 pull requests

- Successful merges: #36679, #36699, #36997, #37040, #37060, #37065, #37072, #37073, #37081
- Failed merges:
@bors bors merged commit 0c2b258 into rust-lang:master Oct 12, 2016
@QuietMisdreavus QuietMisdreavus deleted the rustdoc-line-breaks branch October 12, 2016 01:56
@bluss
Copy link
Member

bluss commented Oct 12, 2016

Nice. Have you considered line breaking long where clauses? 😄 Example function in rustdoc

@QuietMisdreavus
Copy link
Member Author

@bluss sure (^^)b #37190

eddyb added a commit to eddyb/rust that referenced this pull request Nov 11, 2016
…ne, r=GuillaumeGomez

rustdoc: add line breaks to where clauses a la rustfmt

Much like my last PR for rustdoc (rust-lang#36679), this adds line breaks to certain statements based on their line length. Here the focus was on where clauses.

Some examples:
- [Where clause in a trait function](https://shiva.icesoldier.me/custom-std/std/iter/trait.Iterator.html?search=#method.unzip) (also in the trait header block at the top of the page)
- [Where clause on a bare function](https://shiva.icesoldier.me/doc-custom2/petgraph/visit/fn.depth_first_search.html)
- [Where clauses in trait impls on a struct](https://shiva.icesoldier.me/custom-std/std/collections/struct.HashMap.html) (scroll to the bottom) These are regularly not on their own line, but will be given their own line now if their "prefix text" doesn't give them enough room to sensibly print their constraints. HashMap's trait impls provide some examples of both behaviors.

The libstd links above are the whole docs rendered with this, and the "bare function" link above is in another set that pulls some notable crates together. `petgraph` was the one that brought this request up, and that collection also includes [itertools](https://shiva.icesoldier.me/doc-custom2/itertools/trait.Itertools.html) which provided an easy sample to test with.

r? @GuillaumeGomez
eddyb added a commit to eddyb/rust that referenced this pull request Nov 12, 2016
…ne, r=GuillaumeGomez

rustdoc: add line breaks to where clauses a la rustfmt

Much like my last PR for rustdoc (rust-lang#36679), this adds line breaks to certain statements based on their line length. Here the focus was on where clauses.

Some examples:
- [Where clause in a trait function](https://shiva.icesoldier.me/custom-std/std/iter/trait.Iterator.html?search=#method.unzip) (also in the trait header block at the top of the page)
- [Where clause on a bare function](https://shiva.icesoldier.me/doc-custom2/petgraph/visit/fn.depth_first_search.html)
- [Where clauses in trait impls on a struct](https://shiva.icesoldier.me/custom-std/std/collections/struct.HashMap.html) (scroll to the bottom) These are regularly not on their own line, but will be given their own line now if their "prefix text" doesn't give them enough room to sensibly print their constraints. HashMap's trait impls provide some examples of both behaviors.

The libstd links above are the whole docs rendered with this, and the "bare function" link above is in another set that pulls some notable crates together. `petgraph` was the one that brought this request up, and that collection also includes [itertools](https://shiva.icesoldier.me/doc-custom2/itertools/trait.Itertools.html) which provided an easy sample to test with.

r? @GuillaumeGomez
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants