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 signature #547

Closed
wants to merge 2 commits into from
Closed

Format signature #547

wants to merge 2 commits into from

Conversation

Krzysztof-Cieslak
Copy link
Member

This introduces nicely formatted signatures like ones used in the Ionide/VS4Mac/Rider

Current live version:
image

Updated version:
image

I believe with this change we don't need to list parameters and return type separately as it's clear from the signature.

@dsyme
Copy link
Contributor

dsyme commented Jul 20, 2020

Hi, I only just saw this. It will clash a bit with the changes I just pushed in sorry, but I think you'll be able to fit things together.

I like the signature for the hover tips, it looks great

@dsyme
Copy link
Contributor

dsyme commented Jul 20, 2020

@Krzysztof-Cieslak As a separate note, I'd like to go through and make all the type formatting generate HTML instead of strings

However this would mean the currently internalised HtmlModel being used here (note: ripped from Fornax) would be exposed as public.

Since FSharp.Formatting is at the root of the universe I think this is ok? But it could require translation from FSHarp.Formatting HTML --> Fornax HTML model

Do you think that HtmlModel should be in a single component all of its own, with a stable 1.0 version number? I was loathe to take a dependency on Fornax.Core v0.2 - for application code I would, but FSharp.Formatting is at the root of the entire universe and so I really want to avoid any significant dependencies for it.

@baronfel
Copy link
Collaborator

The propagation of html rendering engines in the ecosystem is a big pain point IMO. This, fornax, giraffe, Feliz, falco all have different but similar implementations :-/

@dsyme
Copy link
Contributor

dsyme commented Jul 20, 2020

The propagation of html rendering engines in the ecosystem is a big pain point IMO. This, fornax, giraffe, Feliz, falco all have different but similar implementations :-/

Right

I mean I've internalised it here so far, and it's currently not used in the ApiDocsModel. The use in GenerateHtml is internal.

We could give the structuring of types in the APiDocsModel without it (as a separate data model). I suppose that would be consistent with the current architecture and get the layering right

@dsyme
Copy link
Contributor

dsyme commented Jul 20, 2020

Let's discuss that here: #549

@cartermp
Copy link
Contributor

cartermp commented Aug 6, 2020

@dsyme it's not clear how to proceed with this one now that you've removed SignatureTooltip. Is there a replacement?

@dsyme
Copy link
Contributor

dsyme commented Aug 8, 2020

@dsyme it's not clear how to proceed with this one now that you've removed SignatureTooltip. Is there a replacement?

I think the aim here is to now to put in a new, better SignatureTooltip. But the code will need to be rejigged to produce HTML rather than plain formatted text, I'll see if I can do that (since I'm the cause of most of the conflicts)

BTW we're now at the position that the HTML fragments being generated in ApiDocsModel are limited to:

  1. F# types and signatures
  2. The HTML for individual sections of XML comments (e.g. the HTML formatted for summary, remarks sections)
  3. The HTML for individual sections of Markdown comments

This falls under category (1) so I think it's ok in terms of layering

@cartermp
Copy link
Contributor

Should this PR just be left open, or re-done?

@dsyme
Copy link
Contributor

dsyme commented Aug 19, 2020

Should this PR just be left open, or re-done?

I will assess it, it's on my todo list.

@dsyme
Copy link
Contributor

dsyme commented Oct 11, 2022

Closing this old PR - @Krzysztof-Cieslak Apologies it never got merged. We can look at merging into here of FCS

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