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

Format long signature #782

Merged
merged 15 commits into from
Dec 8, 2021

Conversation

panglesd
Copy link
Collaborator

This PR addresses Issue #565 extended to all signatures.

Similarly to ocamlformat, it uses the Format library with the 80 char rule. The output is then put in a preformatted container in HTML.

There is still work needed to be done for displaying long variants and records, but I think it has already started in #673.

It is also needed to make the latex renderer put DocumentedSrc into preformatted text. I'm not sure if it is in the scope of this PR, as currently the generated latex files do not have a preamble, and I guess this kind of information should be in the preamble.

Finally, some complication arise due to the the Alternative variant of DocumentSrc: sometimes, the formatter do not know in advance how the renderer will choose to render the document, out of the summary and expansion alternative. In this case, the formatter assumes it is the summary alternative that will be chosen.

Example of generated documentation can be found here: tyxml and base.

@dbuenzli
Copy link
Contributor

dbuenzli commented Nov 19, 2021

Similarly to ocamlformat,

In general I wouldn't take ocamlformat as a good example of what to do. Degenerating to one argument per line as soon as you hit the line length somehow assumes you only have one neuron to process text which is insulting. And indeed your results seem to degenerate signatures into one argument per line which is a pure waste of the scarce vertical space.

Also in general on the web you should avoid fixed width layouts. Try to look your results in a narrow viewport (e.g. mobile) they are completely unreadable. They are still readable with the current strategy.

I still think the right strategy is to get a better understanding of the line breaking algorithm and hint breaks more appropriately. So that cut breaks appear at more natural places. A few other things were tried but had to be reverted due to browser usability issues (they either broke cut and paste or broke in page search in one major browser). Given the insane flexiblity of the web stack I'm still pretty confident we can find something that can accomodate fluid layouts in a better manner than this.

@lpw25
Copy link
Contributor

lpw25 commented Nov 19, 2021

I think that one parameter per-line is a good approach to formating long function types. Or at least it is when the parameters are labelled, as they often are in long function types. It is parallel to the formating of record definitions.

I suspect the easiest way to get it looking good on mobile is to work out 80 column and say 40 column formatting and choose between them with a media query. Doing anything cleverer seems to somehow always get stuck on some browser nonsense or other.

@dbuenzli
Copy link
Contributor

I think that one parameter per-line is a good approach to formating long function types. Or at least it is when the parameters are labelled, as they often are in long function types. It is parallel to the formating of record definitions.

For value signatures it's perfectly fine not doing this if you have 1) proper label highlighting for quickly scanning arguments (those are still not spanned and thus not highlighted in any stylesheet out there) and 2) mostly unsurprising break points which still seems quite doable to me. You just need to start in a principled way by defining good atoms which is possible by doing what was done for -> (e.g. to not break on the space of [> `Var]) or not to break on the : of the label:type).

Beyond values there are other problems with e.g. functors and with constraints and these ones do need a few well inserted <br> or block level wrapping elements to distinguish their parts. At the moment they are just dumped regardless which obviously makes them totally unreadable.

This just needs a bit more attention that has ever been given to up to now with a good eye for usability. And as far as the latter is concerned given the amount of pointless scrolling it will incur on most pages, I'm afraid the solution of this PR is really a terrible one.

@panglesd
Copy link
Collaborator Author

In general I wouldn't take ocamlformat as a good example of what to do.

I personnaly find that ocamlformat makes things very readable, with one argument per line, but that’s only preferences. What is important, in my opinion, is to have a unified way of formatting, so that the readers are used to what they see and comfortable with it: it is easy to misinterpret a type if you think it is formatted in the habitual way (all in one line, or one argument per line), but in fact, it is not. Also, I don’t see vertical space as a very scarce ressource, and scrolling as that horrible: In APIs with very long signatures, it is very likely that the reader will use in-page search anyway.

Beyond values there are other problems with e.g. functors and with constraints and these ones do need a few well inserted
or block level wrapping elements to distinguish their parts. At the moment they are just dumped regardless which obviously makes them totally unreadable.

Functors and constraints are formatted, see some tyxml functors

I still think the right strategy is to get a better understanding of the line breaking algorithm and hint breaks more appropriately. So that cut breaks appear at more natural places. A few other things were tried but had to be reverted due to browser usability issues (they either broke cut and paste or broke in page search in one major browser). Given the insane flexiblity of the web stack I'm still pretty confident we can find something that can accomodate fluid layouts in a better manner than this.

I disagree and will give you a few arguments:

  • In my opinion, the behavior should be the following:
    1. If there are enough space, format as close as ocamlformat, as we want the reader to feel comfortable with the formatting.
    2. If there is not enough space, use css and good markup to achieve the best that we can
  • This PR addresses the first case. Moreover, as what it does is just adding white-spaces, and one css line, it does not make case 2 any harder to implement.
  • Case 2 should be addressed in another PR, along many other css rules for narrow screens. For instance, there is by default a margin taking all the horizontal space on narrow screens. There is no point in working very hard for formatting before these simple media queries are implemented, (in another PR)
  • If someone wants to use something else that the default ocaml formatting (aka ocamlformat), this PR does not change anything for him, as it is only adding whitespaces. I guess they would still like to have or make a PR with better markup, but this has nothing to do with this PR.
  1. proper label highlighting for quickly scanning arguments (those are still not spanned and thus not highlighted in any stylesheet out there)

I agree that syntax highlighting is lacking, the only thing we have here is bold keywords. I might try to add some soon.

I'm afraid the solution of this PR is really a terrible one

I'm not sure, why it is terrible?
If it's because of the scrolling, maybe ocamlformat is to blame, as I think it is important to follow the standard. If it's because of narrow screens, I think that should be dealt in another PR, this one does not change anything for that. If it's because preformatted text, even on large screen, is bad for some reason compared to html markup (even when the result looks good) I did not know that and would be happy to hear more about it!

Copy link
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the formatting !

This unfortunately doesn't interact well with the --indent option of the html backend due to these issues in tyxml: ocsigen/tyxml#287 ocsigen/tyxml#288
The HTML tests are not rendering well because of this (dune runtest).

@@ -180,7 +210,11 @@ let rec list ?sep ~f = function
let tl = list ?sep ~f xs in
match sep with None -> hd ++ tl | Some sep -> hd ++ sep ++ tl)

let render f = spf "@[%t@]" (span f)
let box_hv t ppf = pf ppf "@[<hv 2>%t@]" t
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API is getting bigger, it's time to make it more abstract: panglesd#1 (a PR to your PR, feel free to squash)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops I forgot to squash. I can rewrite history if you prefer.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to squash unless you want to :)

@dbuenzli
Copy link
Contributor

I'm not sure, why it is terrible?

It lacks compactness and information density, show the data – grab a Tufte book if you don't understand what that means. Using pre-formatted code is not a good solution, you won't be able to reformat pre-formatted code with a CSS query. Things will be get unreadable on narrow viewports and you are not making a good use of the screen estate, pages will be mostly whitespace.

Layout on the web is not about making it look good on one or two viewport sizes, it is about making it accessible for users on a diversity of viewports. Not everyone out there is reading these pages on 27'' inches screens. On most laptops vertical space is extremely scarce (because you already have the OS + the browser chrome eating a good 10% of the vertical height) and viewports get narrow because you want to be able to have an editor and a web page side-by-side.

Scrolling to access content is extremely annoying for users, in fact if you design websites you'll quickly learn that a lot of users never read beyond the page fold. If the first thing you have to do as a user when you get on a page is to scroll to be able to access the content then your design is just failing at your users.

@jonludlam
Copy link
Member

I think this PR is a vast improvement on what we currently have.

The fact that it looks like ocamlformat output is a good thing. We go to some trouble to make these parts of the docs look like code - we put them in monospaced fonts, we have hidden comment markers so we can cut-n-paste - so formatting and highlighting it so it looks like the very familiar ocamlformat output seems totally reasonable.

Additionally using preformatted text rather than using CSS might not be the ideal solution but you've already pointed out several failed attempts to do it that way - it's definitely an approach that is more likely to have problems. If someone can demonstrate a good CSS-based solution we'd have something concrete to compare with, but right now this solution is here and makes stuff look a lot better than before, so I'm very happy to merge this.

@dbuenzli
Copy link
Contributor

I rather think that in average the PR makes things worse than they were, especially on signatures that looked good before.

If you seriously think that for example for example this a lot better with what was before then there's no point in arguing this further. I'm tired of fighting with people who have little sense of usability and typographic rules.

@Julow
Copy link
Collaborator

Julow commented Nov 24, 2021

Layout on the web is not about making it look good on one or two viewport sizes, it is about making it accessible for users on a diversity of viewports.

Remember that we are showing source code on the web here. This PR is solving a problem, it's not just a styling change: code outputted by Odoc was unreadable, on every screen sizes.

A fair amount of line breaks are needed to make code readable to most people. How do you write a 200-character long value type ?
The rule implemented in this PR is simple enough that it improves reading for me. (can't miss an argument if there's one per line) Also, that it formats similarly to ocamlformat is more comfortable for people using it, and not much worse for others because ocamlformat is not that bad (do you prefer a more complicated rule that is harder to read to most people ?).

@dbuenzli
Copy link
Contributor

This PR is solving a problem, it's not just a styling change: code outputted by Odoc was unreadable, on every screen sizes.

Some of them were unreadable for reasons I already highlighted above. Are you seriously suggesting that the pages output by odoc or ocamldoc for the past twenty years were unreadable in the average case because they weren't dumbed down to one argument/result component per line ?

@samoht
Copy link
Member

samoht commented Nov 24, 2021

@dbuenzli please keep the tone of the discussion friendly. We are all trying to improve things here, especially @panglesd who is a new contributor to the project!

Tone aside:

If you seriously think that for example for example this a lot better with what was before then there's no point in arguing this further.

The result on that example can indeed be improved: for instance, ocamlformat tries to group tuples together on the same line when possible, e.g.:

val partition3_map :
  'a t ->
  f:('a -> [ `Fst of 'b | `Snd of 'c | `Trd of 'd ]) ->
  'b t * 'c t * 'd t

Maybe using something similar would work here?

@Julow
Copy link
Collaborator

Julow commented Nov 24, 2021

If you seriously think that for example for example this a lot better with what was before

This can be considered a bug, fixed here: panglesd#2 (a PR to the PR)

I'm tired of fighting with people who have little sense of usability and typographic rules.

This is a strong assumption. I'm tired of your aggressive tone. You can't just dismiss others opinion by just saying "it's dumb". It's not useful. Either contradict other opinions peacefully with examples and data or leave the decisions to others.

@jonludlam
Copy link
Member

Neat, thanks @samoht and @Julow :-)

Copy link
Contributor

@Drup Drup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@panglesd and I discussed his approach already, but I have now read most of the implementation. I think it's an excellent first step. It's not the omega of documentation indentation, but nothing is ever perfect. ;)

The code and the approach in general is clean and can be improved further incrementally and it's definitely much better than what we had before, except for some minor fixable issues. 👍

The main issue is doesn't yet enable responsiveness ... but one problem at a time.

Small remarks:

  • We can probably improve some code slightly by using a combinator similar to Fmt.any instead of O.txt, which would allow things of the form O.txt " *@ ".
  • The sizing of ignored elements has some cases missing that I suspect will cause issues, notably linebreak. It's probably better to detect when we are going to hit something like that, and handle it properly immediately.
  • Have you checked that everything run smoothly on recent OCaml with the "clean" version of Codefmt.Tag ?

All of this comments can be solved in further patches, so we can merge as soon as @panglesd is ready.

@panglesd
Copy link
Collaborator Author

panglesd commented Dec 3, 2021

Thanks @Drup!

I corrected the semantic tag version of Codefmt.Tag, and tested it this time.
I agree that Linebreak would cause issues with computed sizes. However, it does not seem to be generated on the project...

Merge is ready on my side :)

@jonludlam
Copy link
Member

@panglesd - there's a conflict to resolve before we can merge this.

panglesd and others added 14 commits December 6, 2021 14:44
Signed-off-by: Paul-Elliot <[email protected]>
Signed-off-by: Paul-Elliot <[email protected]>
Signed-off-by: Paul-Elliot <[email protected]>
with an abstract type t. Will be useful for the following changes.
The generator is no longer aware that Codefmt is implemented using
closures. Generator code can no longer execute in during rendering.
Signed-off-by: Paul-Elliot <[email protected]>
Signed-off-by: Paul-Elliot <[email protected]>
Signed-off-by: Paul-Elliot <[email protected]>
@panglesd panglesd force-pushed the format-long-signature-with-format branch from 4cf6cf5 to 34b7c61 Compare December 6, 2021 13:44
@jonludlam jonludlam merged commit 86a7a9e into ocaml:master Dec 8, 2021
jonludlam added a commit to jonludlam/opam-repository that referenced this pull request Feb 8, 2022
CHANGES:

Additions
- New subcommand to resolve references (@panglesd, @lubega-simon, ocaml/odoc#812)
- Improved rendering of long signatures (@panglesd, ocaml/odoc#782)
- Handle comments attached to open statement as floating comment, instead
  of dropping them (@panglesd, ocaml/odoc#797)
- Empty includes (containing entirely shadowed entries) are now hidden (@panglesd, ocaml/odoc#798)

Bugs fixed
- Fix a missing Result constructor during compile. This will cause some
  functor arguments to have different filenames (@jonludlam, ocaml/odoc#795)
- Better memory/disk space usage when handling module alias chains (@jonludlam, ocaml/odoc#799)
- Resolving class-type paths (ie., `val x : #c`) (@jonludlam, ocaml/odoc#809)
- Skip top-level attributes while extracting the top comment. Fix top-comment extraction with PPX preprocessing (@jorisgio, ocaml/odoc#819)
- Better handling of @canonical tags (@jonludlam, ocaml/odoc#820)
- css: improved layout (@jonludlam, @Julow, ocaml/odoc#822)
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.

7 participants