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

Better Completion for ExternalAutocomplete functions #1178

Merged
merged 7 commits into from Oct 20, 2023

Conversation

Tangent-90
Copy link
Contributor

@Tangent-90 Tangent-90 commented Oct 18, 2023

WHAT

🤖 Generated by Copilot at b053be9

This pull request adds a new feature to the F# language server, which allows the user to configure how completion items for external symbols are displayed. It also improves the completion items for hash directives and keywords, and adds tests for the new feature. The main files affected are src/FsAutoComplete.Core/KeywordList.fs, src/FsAutoComplete/LspHelpers.fs, src/FsAutoComplete/LspServers/AdaptiveFSharpLspServer.fs, and test/FsAutoComplete.Tests.Lsp/CompletionTests.fs.

🤖 Generated by Copilot at b053be9

FullNameExternal
Option for completion items
Autumn of symbols

🧑‍💻🔧🧪

WHY

  1. Try fix insert "Array.map" become "Array.map"

Before:
GIF 2023-10-18 23-07-09

After:
GIF 2023-10-18 23-05-58

  1. Fix completions with same label have same description

Before:
GIF 2023-10-19 22-04-52

After:
GIF 2023-10-19 22-08-33

  1. Add FullNameExternalAutocomplete

Before:
GIF 2023-10-18 23-01-10

With FullNameExternalAutocomplete:
GIF 2023-10-18 22-59-03

GIF 2023-10-19 22-09-38

HOW

🤖 Generated by Copilot at b053be9

  • Add a new configuration option for full name external autocomplete, which controls whether the completion items for external symbols should use their full names instead of suggesting to open their namespaces (link, link, link, link, link, link, link, link, link, link, link)
  • Implement the logic for creating and resolving completion items based on the full name external autocomplete option, using the getCodeToInsert function and the data field of the completion items (link, link, link, link, link, link)
  • Add a label field and a data field to the completion items for hash directives and keywords, using the # prefix for directives (link, link, link)
  • Reformat the code for creating and checking completion requests and responses in the test cases, to make it more consistent and readable (link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link)
  • Add a new test list for testing the full name external autocomplete feature, using the fullNameExternalAutocompleteTest function and the Range.FullNameExternalAutocomplete field (link, link)

Add FullNameExternalAutocomplete
(for `module Result` added `open System.Diagnostics.Contracts.Contract`)
fix: FullNameExternalAutocomplete can't get description
fix: completions with same label have same description
add: filter completions by full name under fullNameExternalAutocomplete
add: tests for fullNameExternalAutocomplete
Copy link
Member

@TheAngryByrd TheAngryByrd left a comment

Choose a reason for hiding this comment

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

🎸 This looks great! Gonna let @baronfel make a review pass too.

Kind = (AVal.force glyphToCompletionKind) d.Glyph
InsertText = Some code
InsertText = Some(getCodeToInsert d)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: getCodeToInsert d is calculate above in code - can we reuse it here to prevent recalculating?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 2614 to 2618
| Some no when config.FullNameExternalAutocomplete ->
// allow filter "Ceiling (System.Math)" by "System.Math.Ceiling" or "CeilingSystem.Math"
sprintf "%s (%s)" d.NameInList no, d.NameInList + code
| Some no -> sprintf "%s (open %s)" d.NameInList no, d.NameInList
| None -> d.NameInList, d.NameInList
Copy link
Contributor

Choose a reason for hiding this comment

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

can you describe in a comment the logic going on here? how does the label selection and the filter text impact the user experience of searching/filtering in the IDE?

The previous implementation didn't document why decisions were made, and so we don't have any sort of record for justification for making changes.

@Tangent-90
Copy link
Contributor Author

There's two ways to display conpletions label, one is like "AA.BB.CC" and another is "CC (AA.BB)".
Image_1697796857902.png

Image_1697796867566.png

When the full name is long, comparing to the second display way, the first one seems to be too messy, and can't quickly distinguish member's name.

Image_1697796804752.png

Image_1697796812164.png

So completions label is decided to display like "CC (AA.BB)".

To enter part of the full name to filter completion items, the filter text must contain its full name "AA.BB.CC".

As completions member name was the first part of the label, to make it first filter out by member name, and to make filtered label highlighted correctly by editors, the name must to be the first part of the filter text.

As the result, the format of filter text is member name + full name, which is "CCAA.BB.CC".

@baronfel
Copy link
Contributor

Thanks @Tangent-90 - that helps me understand the intent quite a bit more!

@baronfel
Copy link
Contributor

Pushed a tiny change to add more to the docs so it was clearer to me personally, but otherwise this LGTM and let's ship it! Ideally we could also add the option to the Ionide package.json so that end users have some docs for it.

@baronfel baronfel enabled auto-merge (squash) October 20, 2023 15:53
@baronfel baronfel merged commit 7f88622 into ionide:main Oct 20, 2023
8 of 9 checks passed
TheAngryByrd pushed a commit that referenced this pull request Oct 21, 2023
Co-authored-by: Chet Husk <chusk3@gmail.com>
@TheAngryByrd TheAngryByrd changed the title Change to ExternalAutocomplete functions Better Completion for ExternalAutocomplete functions Oct 28, 2023
nojaf pushed a commit to nojaf/FsAutoComplete that referenced this pull request Nov 3, 2023
Co-authored-by: Chet Husk <chusk3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants