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

[WIP] Experiment with porting generation to signature like API #932

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

MangelMaxime
Copy link

@MangelMaxime MangelMaxime commented Jul 13, 2024

Please do not merge as more work is needed before merging into main

This PR experiment with how we could use signature to document the API like proposed in #929.

🚨 I believe the CI is failing because I touch some templates files or because the HTML generation changed. It will be fixed once things are more stable.

Edit: Forgot to add a preview

CleanShot.2024-07-14.at.17.27.08.mp4

Improve XLM Doc formatting

We use a port of TipFormatter.fs from FsAutoComplete.

I need to update it to the latest version, because I am using a fork from 3 years ago but it should not be too difficult.

Rendering of Signature

A new file GenerateSignature.fs has been created to isolate the logic used to render signatures. This is done using a custom TextNode DSL which is responsible for both rendering HTML but also computing the size in number of characters used by the signature elements.

This is important to be able to align the different signature elements:

CleanShot 2024-07-14 at 16 16 08

The main difficulty here came from FSharp.Formatting.HtmlModel because it renders the HTML with a lot of indentations. These indentations causes issues because some of them are interpreted as spaces by the browsers.

To work around this problem, I add a new this.ToMinifiedHtml() method.

I believe by default F# formatting should probably always minimise the generated HTML for optimising the size. If needed, for the tests it is possible to invoke an HTML beautifier like prettier which can be invoked via CLI.

Remarks regarding the current CSS

Note

The CSS I introduce is not optimised, I just added to for making things look ok for the experimentation

While working on this PR, I had a look at the CSS used and have several remarks:

  • I think it could be an improvement to use the BEM convention to harmonise the CSS naming convention in the repertoire.

  • It is recommended use rem and em instead of px for controlling the size in CSS.

    This is because rem and em are better for scaling and respect the user default font.

  • When designing a page layout, now days it is preferred to use margin-bottom instead of margin-top. This is because it follows the default flow of the page (top to bottom) and can be read as. After each paragraph, I add some spacing unless I am the last element of my scope for example.

  • The number of variables should be kept minimal if possible.

    For example, some of the variables are too contextual to be included by default IHMO, for example the having a spacing of 96px, 128px, 192px are probably never going to be used except for a specific components which will be able to define it's spacing for itself.

    This makes user decision easier because they have less options for the default styling.

  • Regarding the name of the CSS variables, I recently explored using the standard SI units instead of obscures names like main, second, important, etc. which don't works in all the contexts and for which the relation between them is not clear.

    /* Font sizes */
    --font-size-pico: 0.625rem;
    --font-size-micro: 0.75rem;
    --font-size-milli: 0.875rem;
    --font-size-base: 1rem;
    --font-size-kilo: 1.25rem;
    --font-size-mega: 1.5rem;
    --font-size-giga: 2rem;

    /* Spacing */
    --spacing-000: 0; /* 0px */
    --spacing-femto: 0.125rem; /* 2px */
    --spacing-pico: 0.25rem; /* 4px */
    --spacing-nano: 0.5rem; /* 8px */
    --spacing-micro: 0.75rem; /* 12px */
    --spacing-base: 1rem; /* 16px */
    --spacing-kilo: 1.5rem; /* 24px */
    --spacing-mega: 2rem; /* 32px */
    --spacing-giga: 3rem; /* 48px */

Prism.css needs to be customised to use the CSS variables used by F# formatting.

How was the colour for the syntax highlighting chosen? I am asking because I find some of the colour strange for a theme. We could perhaps looks at existing VSCode theme like Atom One or Vs and use their settings. This will make it easier for us to have good contrast for light and dark theme.

F# Formatting

I think it would be a good idea to revisit the Fantomas configuration especially regarding the list formatting.

I believe the current setting is fsharp_multiline_bracket_style = cramped which cause a lot of weird indentation (2 spaces instead of 4 spaces).

This makes the code harder to edit and copy/pasting difficult because the indentation is most often incorrect in the new place.

See how in the screenshot below almost none of the code is indented on the default 4 spaces indentation.

CleanShot 2024-07-14 at 16 37 02

Using fsharp_multiline_bracket_style = aligned make a huge improvements because everything is aligned on a 4 spaces "grid".

To go even further, we could also experiment with using a Feliz like syntax for HTML instead of the double list. But this will requires more work, and I don't know the opinion of everyone on using this syntax. For people, who don't know what I am talking about here is a "blog post" that I wrote which compares both approach.

Plus, I think using fsharp_multiline_bracket_style = aligned gives us a big improvements in term of quality of life for a minimal efforts thanks to the work done by Fantomas team.

Tests

I didn't add any tests, but I think it would be a good idea to add tests to prevents regression in the long run. At minimal, we should adds tests for covering the signature generation and to do so we can use Verify which supports snapshots this avoids having to manually maintains the expected strings which can be quite complexes.

Conclusion

In the end, I don't think the changes needed to use signature for generating the documentation are not too complex. It offers the opportunity to restructure the old code, to make it easier to read and maintain in the long run.

Please feel free to ask questions and provide feedbacks on the initial works done, and I will do my best to adapt it and convert all the generation to the new proposition.

@MangelMaxime MangelMaxime marked this pull request as draft July 13, 2024 17:41
@nhirschey
Copy link
Collaborator

@MangelMaxime just an FYI in case it's useful. Not trying to direct you one way or another, there was a prior attempt at signatures that got abandoned when branches got out of sync: #547

Again, just FYI in case there's something useful there.

@MangelMaxime
Copy link
Author

@nhirschey Thanks for pointing out.

Looks like the problem reported with F# Formatting introducing whitespace where we don't want was there too.

The main difference I see beside the code used to render the signature, is that they focus on adding it in the tooltip while in my proposition we don't need tooltip anymore. This also allows to have part of the signature easily clickable for example, you can click Promise from Promise<'T> to be redirected to Fable.Core.JS.Promise documentation (the URL needs to be fixed or the documentation of dependencies generated too).

@nhirschey
Copy link
Collaborator

nhirschey commented Jul 15, 2024

while in my proposition we don't need tooltip anymore.

I might be misunderstanding what you mean, but even if we don't need the signature tooltip in ApiDocs anymore we'd still use the signature tooltip in the code blocks of literate docs files like these. I'd probably say keep the signature tooltip in both literate docs and apidocs.

Looks like the problem reported with F# Formatting introducing whitespace

Yeah, the whitespace is a pain to keep track of in various tests. Agreed it could be nicer. You may see in the tests various adhoc functions to strip white space.

We're targetting the commonmark spec when converting markdown to html. I'm not sure how minified html affects that, but worth keeping in mind.

@MangelMaxime
Copy link
Author

I might be misunderstanding what you mean, but even if we don't need the signature tooltip in ApiDocs anymore we'd still use the signature tooltip in the code blocks of literate docs files like these. I'd probably say keep the signature tooltip in both literate docs and apidocs.

Signature tooltips are kept in the literate docs.

For the API docs, we don't need to keep them because all the information is already in the new signature and documentation.

One thing to decide, could be if we want to always show all the information like Elm:

CleanShot 2024-07-15 at 15 26 40

Show a portion of it and add a ReadMore button like Rust:

CleanShot 2024-07-15 at 15 27 14

Make it fully collapsed like VSCode API:

CleanShot 2024-07-15 at 15 27 51

CleanShot 2024-07-15 at 15 18 14

CleanShot 2024-07-15 at 15 19 10

We're targetting the commonmark spec when converting markdown to html. I'm not sure how minified html affects that, but worth keeping in mind.

I believe that Markdown is not be affected by having a minified or indented HTML.

@nojaf
Copy link
Collaborator

nojaf commented Aug 5, 2024

Hi there, thanks for this initial attempt! Here are some preliminary remarks; I haven't looked into the details yet.

Improve XML Doc formatting

I'd like to hear the strategy for how Ionide and FSDocs can use the exact same code base. Simply updating the code and keeping a copy doesn't seem like a good idea. Please go the extra mile on this.

Rendering of Signature

The way the signature is formatted does look quite different from anything I've seen before. I'm not against it, but I'd feel more comfortable if others could weigh in and confirm that this is truly an improvement. It's a departure from the style guide, which isn't necessarily bad, but it should be a very conscious decision. @dsyme, any thoughts on this?

Remarks regarding the current CSS

he CSS approach was inspired by https://www.refactoringui.com/
The book goes into detail why px is used over em/rem.
And some of the other choices that were made were inspired by this book.

In the book "Refactoring UI," the authors recommend using px over rem or em for several reasons:

Consistency: px units provide a consistent size across different elements and contexts. This consistency makes it easier to design and maintain a cohesive look and feel throughout the application.

Predictability: px units are more predictable because they do not depend on the font size of parent elements or the root element. This predictability simplifies the design process and reduces unexpected changes in element sizes due to changes in font sizes.

Ease of Design: Designers typically think in terms of pixels when creating mockups and layouts. Using px units allows for a more straightforward translation from design tools to code, ensuring that the implemented design matches the original intention.

Simplified Calculations: Using px units can simplify the calculations needed for spacing, alignment, and sizing. With rem or em, you often have to consider the inheritance and cascading of font sizes, which can complicate these calculations.

Avoiding Accessibility Issues: While rem and em units are often recommended for their scalability and accessibility benefits (like respecting user font size preferences), the book suggests that modern browsers and devices handle pixel-based layouts well enough that using px does not inherently cause accessibility issues, especially if other best practices for accessibility are followed.

The recommendation is based on the practical aspects of designing and maintaining a UI, emphasizing simplicity and reliability in the design-to-code workflow.

There was a specific period for giving feedback on the CSS of the modern theme, from November to February. For me, that window has closed, and I prefer not to revisit it just because a new contributor might have done things differently. This is a common scenario in open source projects, and most of your comments seem to reflect a different perspective rather than actual issues. Discussing this with every new contributor isn't productive, so I don't want to reopen these topics as part of this PR. However, you should be able to contribute your ideas with only localized changes.

How was the colour for the syntax highlighting chosen?

A bit random, I would be ok with revisiting that.

Fantomas configuration

I'm not against revisiting the Fantomas configuration. Feel free to propose some changes in a separate PR, and the others can weigh in on whether it's worth making those changes.

Feliz like syntax for HTML

I'm personally a fan of the two lists DSL; it mimics the XML/JSX structure with attributes and children nicely. This isn't a problem that needs addressing, just a personal preference. There's no need to update the DSL for your changes, so please make it work with what we have.

Tests

Some regression tests at the end would be nice. It would be great if they could cover everything possible with XML comments. I often struggle to reproduce certain issue reports because I'm less aware of where things are coming from.

@nhirschey
Copy link
Collaborator

rendering of signature.

I don't have strong feelings but I'd probably stick with the style guide. My interpretation of the style guide is the rendering should be

val eitherEnd:
    success: 'T -> unit ->
    fail: exn -> unit ->
    pr: 'T ->
        unit

@MangelMaxime
Copy link
Author

MangelMaxime commented Aug 12, 2024

Remarks regarding the current CSS

he CSS approach was inspired by refactoringui.com The book goes into detail why px is used over em/rem. And some of the other choices that were made were inspired by this book.

Thanks for the information, I will have a look at it because this the first time I see px being recommended over rem.

And like you said, there was a window for CSS feedback that was missed. In theory, because F# formatting allows custom templates hopefully it means that if someone wants it is possible to change everything to their liking.

Feliz like syntax for HTML

I'm personally a fan of the two lists DSL; it mimics the XML/JSX structure with attributes and children nicely. This isn't a problem that needs addressing, just a personal preference. There's no need to update the DSL for your changes, so please make it work with what we have.

I have the same idea on this point indeed. And improving the Fantomas configuration will already be a huge saver.

Tests

Some regression tests at the end would be nice. It would be great if they could cover everything possible with XML comments. I often struggle to reproduce certain issue reports because I'm less aware of where things are coming from.

I do have a bunch of tests available for the XML comment formatter in Nacara which I will be able to port over in this PR or in the re-usable library for both FSAC and F# Formatting (or any F# project actually).

Rendering of Signature

The way the signature is formatted does look quite different from anything I've seen before. I'm not against it, but I'd feel more comfortable if others could weigh in and confirm that this is truly an improvement. It's a departure from the style guide, which isn't necessarily bad, but it should be a very conscious decision. @dsyme, any thoughts on this?

Something to note here is based on the snippet provided by @nhirschey

val eitherEnd:
    success: 'T -> unit ->
    fail: exn -> unit ->
    pr: 'T ->
        unit

Then generating this kind of signature is easier because we don't need to know what is the max width of the different elements to keep the alignement.

I personally prefer the aligned version of the signature for documentation because it easier on the eyes thanks to blocking. And also because it is done automatically, I would not do that inside of my code as I don't want to spend time manually indenting stuff 😅

Now that I gathered several feedbacks and I am back from vacation, I will continue working on this PR.

@MangelMaxime
Copy link
Author

Coming back regarding the Signature format, I just remembered that I took inspiration on how Ionide renders it:

CleanShot 2024-08-13 at 15 26 11

.editorconfig Outdated
[{GenerateHtml.fs,tests/FSharp.ApiDocs.Tests/files/ReferenceProject/**/*.fs}]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer a separate PR for this.

Copy link
Author

Choose a reason for hiding this comment

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

Sure will do, just wanted to have it at least for me when working locally because I was too frustrated with dealing with indentation manually 😅

@@ -93,4 +93,38 @@ pipeline "Verify" {
runIfOnlySpecified true
}

let referenceProjectDir = "./tests/FSharp.ApiDocs.Tests/files/ReferenceProject"

// TODO: Revisit to see how we can use `dotnet watch` to run a local version of the tool
Copy link
Collaborator

Choose a reason for hiding this comment

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

This worked for me:

dotnet watch run --project .\src\fsdocs-tool\fsdocs-tool.fsproj -- watch --input "$HOME/projects/fantomas/docs" --eval --projects "$HOME/projects/fantomas/src/Fantomas.Core/Fantomas.Core.fsproj"

(you probably don't need --eval)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the hint I will have a look.

@brianrourkeboll
Copy link
Member

#932 (comment)

rendering of signature.

I don't have strong feelings but I'd probably stick with the style guide. My interpretation of the style guide is the rendering should be

val eitherEnd:
    success: 'T -> unit ->
    fail: exn -> unit ->
    pr: 'T ->
        unit

#932 (comment)

I personally prefer the aligned version of the signature for documentation because it easier on the eyes thanks to blocking. And also because it is done automatically, I would not do that inside of my code as I don't want to spend time manually indenting stuff 😅

#932 (comment)

Coming back regarding the Signature format, I just remembered that I took inspiration on how Ionide renders it:

CleanShot 2024-08-13 at 15 26 11

For what it's worth, I also subjectively find the aligned signatures easier to read (especially for longer or more complex ones), and I think that they are a good fit for automatically generated documentation like this, even though they differ from the style guide.

I always appreciate Ionide's (aligned) signature tooltips every time I use it instead of VS (not a completely fair comparison, since the style guide does at least allow for indenting params, while VS tooltips just wrap arbitrarily).

The style guide's main motivation for not recommending aligning signatures in source code — minimizing diffs/manual realignment — doesn't apply here, and the added readability for something whose sole purpose is to be read seems (to me) more important than strict consistency with the style guide.

But I can understand not wanting to add a new inconsistency, however small and justified, to a core library like this.

@nhirschey
Copy link
Collaborator

Good feedback @brianrourkeboll, thank you. I’m fine with alignment, but I’d say stick with style-guide suggestion that all lines end with ->, particularly the last line.

use

pr: 'T ->
        unit 

Instead of

pr: 'T
        -> unit 

I assume the style guide made that point about -> for a reason.

All that said, whatever ionide does carries weight for sure. The signature format of ionide is the way many people are used to seeing them and will get interpreted as the de facto standard.

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.

4 participants