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

Use of the now public Range propery of PreXmlDoc #1128

Merged
merged 2 commits into from
Jul 4, 2023

Conversation

dawedawe
Copy link
Contributor

@dawedawe dawedawe commented Jul 2, 2023

WHAT

🤖 Generated by Copilot at e5b624e

The pull request optimizes the code that handles XML documentation for F# symbols in two files: AddMissingXmlDocumentation.fs and ConvertTripleSlashCommentToXmlTaggedDoc.fs. It avoids unnecessary computations and simplifies the access to the range of the PreXmlDoc nodes.

🤖 Generated by Copilot at e5b624e

Oh we are the coders of the F# sea
And we write the docs in XML
But we don't waste our time or memory
We use xd.Range and PreXmlDoc well

🚀📝🧹

WHY

Some time ago, the Range property of PreXmlDoc was made public. After updating fsc, we can now make use of it to save resources in some range checks.

HOW

🤖 Generated by Copilot at e5b624e

  • Optimize the computation of XmlDoc by moving it inside the if branch that checks the position range of the PreXmlDoc (link, link)
  • Simplify the access to the position range by using xd.Range instead of d.Range in AddMissingXmlDocumentation.fs and ConvertTripleSlashCommentToXmlTaggedDoc.fs (link, link, link)

if Array.isEmpty d.UnprocessedLines then
None
elif d.UnprocessedLines |> Array.exists (fun s -> s.Contains("<summary>")) then
Some(d.UnprocessedLines, d.Range)
Some(d.UnprocessedLines, xd.Range)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

use xd for consistency

Choose a reason for hiding this comment

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

isn't xd.Range a mistake, xd is not defined.

The above variable is still defined as d.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See line 17, xd is the PreXmlDoc passed to the function.

else
let lines =
match d.UnprocessedLines with
| [||] -> [| " <summary></summary>" |]
| [| c |] -> [| $" <summary>%s{c.Trim()}</summary>" |]
| cs -> [| yield " <summary>"; yield! cs; yield " </summary>" |]

Some(lines, d.Range)
Some(lines, xd.Range)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

use xd for consistency

@TheAngryByrd TheAngryByrd merged commit 6cfa454 into ionide:nightly Jul 4, 2023
@dawedawe dawedawe deleted the use_prexmldoc_range branch July 4, 2023 12:06
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