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

Allow user-set ids for xmldoc example nodes #704

Merged
merged 3 commits into from
Oct 30, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/FSharp.Formatting.ApiDocs/GenerateHtml.fs
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ type HtmlRender(model: ApiDocModel) =

for e in m.Comment.Examples do
h5 [Class "fsdocs-example-header"] [!! "Example"]
p [Class "fsdocs-example"] [embed e]
p [Class "fsdocs-example"; if e.Id.IsSome then Id e.Id.Value ] [embed e]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

id's generated in the html now, yay


//if m.IsObsolete then
// obsoleteMessage m.ObsoleteMessage
Expand Down
91 changes: 64 additions & 27 deletions src/FSharp.Formatting.ApiDocs/GenerateModel.fs
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,20 @@ module internal Utils =
module Html =
let sepWith s l = l |> List.sepWith (!! s) |> span []

type System.Xml.Linq.XElement with
member x.TryAttr (attr: string) =
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added this for convenience and then only used it in the new code, but I'd of course be willing to reuse it in the existing attribute fetches in this file if you like.

let a = x.Attribute (XName.Get attr)
if a = null then None
else if String.IsNullOrEmpty a.Value then None
else Some a.Value

/// Represents some HTML formatted by model generation
type ApiDocHtml(html: string) =
type ApiDocHtml(html: string, id: string option) =

/// Get the HTML text of the HTML section
member _.HtmlText = html
/// Get the Id of the element when rendered to html, if any
member _.Id= id

/// Represents a documentation comment attached to source code
type ApiDocComment(summary, remarks, parameters, returns, examples, notes, exceptions, rawData) =
Expand Down Expand Up @@ -110,7 +119,7 @@ type ApiDocComment(summary, remarks, parameters, returns, examples, notes, excep
/// The raw data of the comment
member _.RawData: KeyValuePair<string, string> list = rawData

static member internal Empty = ApiDocComment(ApiDocHtml(""), None, [], None, [], [], [], [])
static member internal Empty = ApiDocComment(ApiDocHtml("", None), None, [], None, [], [], [], [])

/// Represents a custom attribute attached to source code
type ApiDocAttribute(name, fullName, constructorArguments, namedConstructorArguments) =
Expand Down Expand Up @@ -255,7 +264,7 @@ type ApiDocMember (displayName: string, attributes: ApiDocAttribute list, entity
comment: ApiDocComment, symbol: FSharpSymbol, warn) =

let (ApiDocMemberDetails(usageHtml, paramTypes, returnType, modifiers, typars, baseType, location, compiledName)) = details

let m = defaultArg symbol.DeclarationLocation range0
// merge the parameter docs and parameter types
let parameters =
let paramTypes =
Expand All @@ -267,7 +276,7 @@ type ApiDocMember (displayName: string, attributes: ApiDocAttribute list, entity
(psym, pnm, _pnameText, _pty) ]
let tnames = Set.ofList [ for (_psym, pnm, _pnameText, _pty) in paramTypes -> pnm ]
let tdocs = Map.ofList [ for pnm, doc in comment.Parameters -> Some pnm, doc ]
let m = defaultArg symbol.DeclarationLocation range0

if warn then
for (pn, _pdoc) in comment.Parameters do
if not (tnames.Contains (Some pn)) then
Expand All @@ -286,6 +295,30 @@ type ApiDocMember (displayName: string, attributes: ApiDocAttribute list, entity
ParameterType=pty
ParameterDocs=tdocs.TryFind pnm |} ]

do
let knownExampleIds =
comment.Examples
|> List.choose (fun x -> x.Id)
|> List.countBy id
for (id, count) in knownExampleIds do
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this pass checks for duplicate example ids on a per-member basis

if count > 1 then
if warn
then
printfn "%s(%d,%d): warning: duplicate id for example '%s'" m.FileName m.StartLine m.StartColumn id
else
printfn "%s(%d,%d): error: duplicate id for example '%s'" m.FileName m.StartLine m.StartColumn id
for (id, _count) in knownExampleIds do
if id.StartsWith "example-" then
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this pass checks for missing example ids where we potentially inserted a dummy id

Copy link
Contributor

Choose a reason for hiding this comment

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

then on same line as if please per style guide

let potentialInteger = id.["example-".Length..]
match System.Int32.TryParse potentialInteger with
| true, id ->
if warn
then
printfn "%s(%d,%d): warning: automatic identifer generated for example '%d'. Consider adding an explicit example id attribute." m.FileName m.StartLine m.StartColumn id
else
printfn "%s(%d,%d): error: automatic identifer generated for example '%d'. Consider adding an explicit example id attribute." m.FileName m.StartLine m.StartColumn id
| _ -> ()


/// The member's modifiers
member x.Modifiers : string list = modifiers
Expand Down Expand Up @@ -857,7 +890,7 @@ module internal TypeFormatter =

type TypeFormatterParams = CrossReferenceResolver

let convHtml (html: HtmlElement) = ApiDocHtml(html.ToString())
let convHtml (html: HtmlElement) = ApiDocHtml(html.ToString(), None)

/// We squeeze the spaces out of anything where whitespace layout must be exact - any deliberate
/// whitespace must use &#32;
Expand All @@ -866,7 +899,7 @@ module internal TypeFormatter =
/// adding spaces which are actually significant when formatting F# type information.
let codeHtml html =
let html = code [] [html]
ApiDocHtml(html.ToString().Replace(" ", "").Replace("\n","").Replace("\r","").Replace("<ahref", "<a href"))
ApiDocHtml(html.ToString().Replace(" ", "").Replace("\n","").Replace("\r","").Replace("<ahref", "<a href"), None)

let formatSourceLocation (urlRangeHighlight : Uri -> int -> int -> string) (sourceFolderRepo : (string * string) option) (location : range option) =
location |> Option.bind (fun location ->
Expand Down Expand Up @@ -1044,7 +1077,7 @@ module internal SymbolReader =

static member internal Create
(publicOnly, assembly, map, sourceFolderRepo, urlRangeHighlight, mdcomments, urlMap,
assemblyPath, fscOptions, formatAgent, substitutions, warn ) =
assemblyPath, fscOptions, formatAgent, substitutions, warn) =

{ PublicOnly=publicOnly
Assembly = assembly
Expand All @@ -1057,7 +1090,7 @@ module internal SymbolReader =
AssemblyPath = assemblyPath
CompilerOptions = fscOptions
FormatAgent = formatAgent
Substitutions = substitutions}
Substitutions = substitutions }

let inline private getCompiledName (s : ^a when ^a :> FSharpSymbol) =
let compiledName = (^a : (member CompiledName : string) (s))
Expand Down Expand Up @@ -1385,12 +1418,12 @@ module internal SymbolReader =
(remarks |> List.collect (snd >> List.rev) |> List.tailOrEmpty) @
(rest |> List.collect (snd >> List.rev))

let summary = ApiDocHtml(Literate.ToHtml(doc.With(paragraphs=summary )))
let remarks = if remarks.IsEmpty then None else Some (ApiDocHtml(Literate.ToHtml(doc.With(paragraphs=remarks))))
let summary = ApiDocHtml(Literate.ToHtml(doc.With(paragraphs=summary )), None)
let remarks = if remarks.IsEmpty then None else Some (ApiDocHtml(Literate.ToHtml(doc.With(paragraphs=remarks)), None))
//let exceptions = [ for e in exceptions -> ApiDocHtml(Literate.ToHtml(doc.With(paragraphs=[e]))) ]
let notes = [ for e in notes -> ApiDocHtml(Literate.ToHtml(doc.With(paragraphs=e))) ]
let examples = [ for e in examples -> ApiDocHtml(Literate.ToHtml(doc.With(paragraphs=e))) ]
let returns = if returns.IsEmpty then None else Some (ApiDocHtml(Literate.ToHtml(doc.With(paragraphs=returns))))
let notes = [ for e in notes -> ApiDocHtml(Literate.ToHtml(doc.With(paragraphs=e)), None) ]
let examples = [ for e in examples -> ApiDocHtml(Literate.ToHtml(doc.With(paragraphs=e)), None) ]
let returns = if returns.IsEmpty then None else Some (ApiDocHtml(Literate.ToHtml(doc.With(paragraphs=returns)), None))

ApiDocComment(summary=summary, remarks=remarks, parameters=[], returns=returns, examples=examples, notes=notes,
exceptions=[], rawData=raw)
Expand Down Expand Up @@ -1462,7 +1495,6 @@ module internal SymbolReader =

let readXmlCommentAsHtmlAux summaryExpected (urlMap: CrossReferenceResolver) (doc: XElement) (cmds: IDictionary<_, _>) =
let rawData = new Dictionary<string, string>()

// not part of the XML doc standard
let nsels =
let ds = doc.Elements(XName.Get "namespacedoc")
Expand All @@ -1479,19 +1511,19 @@ module internal SymbolReader =
let n = if id = 0 then "summary" else "summary-" + string id
rawData.[n] <- e.Value
readXmlElementAsHtml true urlMap cmds html e
ApiDocHtml(html.ToString())
ApiDocHtml(html.ToString(), None)
else
let html = new StringBuilder()
readXmlElementAsHtml false urlMap cmds html doc
ApiDocHtml(html.ToString())
ApiDocHtml(html.ToString(), None)

let paramNodes = doc.Elements(XName.Get "param") |> Seq.toList
let parameters =
[ for e in paramNodes do
let paramName = e.Attribute(XName.Get "name").Value
let phtml = new StringBuilder()
readXmlElementAsHtml true urlMap cmds phtml e
let paramHtml = ApiDocHtml(phtml.ToString())
let paramHtml = ApiDocHtml(phtml.ToString(), None)
paramName, paramHtml ]

for e in doc.Elements(XName.Get "exclude") do
Expand All @@ -1515,7 +1547,7 @@ module internal SymbolReader =
let n = if id = 0 then "remarks" else "remarks-" + string id
rawData.[n] <- e.Value
readXmlElementAsHtml true urlMap cmds html e
ApiDocHtml(html.ToString()) |> Some
ApiDocHtml(html.ToString(), None) |> Some
else
None

Expand All @@ -1527,7 +1559,7 @@ module internal SymbolReader =
let n = if id = 0 then "returns" else "returns-" + string id
rawData.[n] <- e.Value
readXmlElementAsHtml true urlMap cmds html e
Some (ApiDocHtml(html.ToString()))
Some (ApiDocHtml(html.ToString(), None))
else
None

Expand All @@ -1547,23 +1579,28 @@ module internal SymbolReader =
match urlMap.ResolveCref cname with
| Some reference ->
let html = new StringBuilder()
rawData.["exception-" + reference.NiceName] <- reference.ReferenceLink
let referenceLinkId = "exception-" + reference.NiceName
rawData.[referenceLinkId] <- reference.ReferenceLink
readXmlElementAsHtml true urlMap cmds html e
reference.NiceName, Some reference.ReferenceLink, ApiDocHtml(html.ToString())
reference.NiceName, Some reference.ReferenceLink, ApiDocHtml(html.ToString(), None)
| _ ->
let html = new StringBuilder()
readXmlElementAsHtml true urlMap cmds html e
cname, None, ApiDocHtml(html.ToString())
cname, None, ApiDocHtml(html.ToString(), None)
]

let examples =
let exampleNodes = doc.Elements(XName.Get "example") |> Seq.toList
[ for (id, e) in List.indexed exampleNodes do
let html = new StringBuilder()
let n = if id = 0 then "example" else "example-" + string id
rawData.[n] <- e.Value
let exampleId =
match e.TryAttr "id" with
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tied to the question in the RFC change, what level of uniqueness should be enforced by tooling?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally fail if --strict is used, and warn otherwise

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm noodling around the implementation a bit here still for validation purposes: if the scope is just within a particular example then that's easy to do validation on, because we have all the examples locally, but at any other scope than that things get hard because we don't have that data available and/or we'd have to push the validation up a level...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

settled on duplicate within a member at the moment, but we could expand that to the type semi-easily if desired

| None ->
if id = 0 then "example" else "example-" + string id
| Some attrId -> attrId
rawData.[exampleId] <- e.Value
readXmlElementAsHtml true urlMap cmds html e
ApiDocHtml(html.ToString()) ]
ApiDocHtml(html.ToString(), Some exampleId) ]

let notes =
let noteNodes = doc.Elements(XName.Get "note") |> Seq.toList
Expand All @@ -1573,7 +1610,7 @@ module internal SymbolReader =
let n = if id = 0 then "note" else "note-" + string id
rawData.[n] <- e.Value
readXmlElementAsHtml true urlMap cmds html e
ApiDocHtml(html.ToString()) ]
ApiDocHtml(html.ToString(), None) ]

// put the non-xmldoc sections into rawData
doc.Descendants ()
Expand Down Expand Up @@ -1602,7 +1639,7 @@ module internal SymbolReader =
comment, nsels

let combineHtml (h1: ApiDocHtml) (h2: ApiDocHtml) =
ApiDocHtml(String.concat "\n" [h1.HtmlText; h2.HtmlText])
ApiDocHtml(String.concat "\n" [h1.HtmlText; h2.HtmlText], None)

let combineHtmlOptions (h1: ApiDocHtml option) (h2: ApiDocHtml option) =
match h1, h2 with
Expand Down
13 changes: 13 additions & 0 deletions tests/FSharp.ApiDocs.Tests/ApiDocsTests.fs
Original file line number Diff line number Diff line change
Expand Up @@ -617,6 +617,19 @@ let ``ApiDocs test FsLib1`` (format:OutputFormat) =

files.ContainsKey (sprintf "fslib-test_omit.%s" format.Extension) |> shouldEqual false

[<Test>]
let ``ApiDocs test examples`` () =
let library = testBin </> "FsLib2.dll" |> fullpath

let files = generateApiDocs [library] OutputFormat.Html false "FsLib2_examples"

let testFile = sprintf "fslib-commentexamples.%s" OutputFormat.Html.Extension

files.ContainsKey testFile |> shouldEqual true
let content = files.[testFile]
content.Contains "has-id" |> shouldEqual true


// -------------------Indirect links----------------------------------
[<Test>]
[<TestCaseSource("formats")>]
Expand Down
20 changes: 19 additions & 1 deletion tests/FSharp.ApiDocs.Tests/files/FsLib2/Library2.fs
Original file line number Diff line number Diff line change
Expand Up @@ -127,4 +127,22 @@ module ``Space-Missing`` =
/// Implicit cast operator test
type ``Implicit-Cast``(value: int) = class end
with static member op_Implicit (source: int) : ``Implicit-Cast`` = ``Implicit-Cast``(source)



module CommentExamples =
/// <summary>this does the thing</summary>
/// <example>this is an example
/// </example>
let dothing() = ()

/// <summary>this does the thing</summary>
/// <example id="has-id">this is an example with an id
/// </example>
let dothing2() = ()

/// <summary>this does the thing</summary>
/// <example id="double-id">this is an example with an id
/// </example>
/// <example id="double-id">this is an example with an id
/// </example>
let doubleExampleId() = ()