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.

Sign up for GitHub

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

Jump to bottom

Code completion: named union case fields rule #500

Merged
merged 19 commits into from
Jun 28, 2023
Merged

Code completion: named union case fields rule #500

merged 19 commits into from
Jun 28, 2023

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Mar 17, 2023

This is a clean-up version of https://amplifying-fsharp.github.io/sessions/2023/03/17/.
Thank you again @auduchinok for all the help!

auduchinok
auduchinok reviewed Mar 23, 2023
View reviewed changes
Copy link
Member

@auduchinok auduchinok left a comment

Choose a reason for hiding this comment

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

@nojaf Let's simplify it back to the first field case and support the subsequent fields case separately, when the parser recovery is improved? The getFieldsAfterSemicolon logic is a bit too brittle.

@nojaf
Copy link
Contributor Author

nojaf commented Mar 23, 2023

Fine by me. That sounds very reasonable.

@En3Tho
Copy link
Contributor

En3Tho commented Mar 23, 2023

Looking at all these nested matches... I wonder if you guys (@auduchinok and @nojaf) could somehow breath life into fsharp/fslang-suggestions#968 once again?

@nojaf nojaf force-pushed the named-union-case-fields-rule-experiment branch from 6d8f1d9 to 7ff1349 Compare March 28, 2023 07:07
@nojaf nojaf changed the base branch from net231 to net232 March 28, 2023 07:08
@nojaf
Copy link
Contributor Author

nojaf commented Mar 28, 2023

@En3Tho I have no desire to pursue that suggestion.

@nojaf nojaf force-pushed the named-union-case-fields-rule-experiment branch from 7ff1349 to 5d6e4a3 Compare June 21, 2023 09:18
@nojaf
Copy link
Contributor Author

nojaf commented Jun 21, 2023

@auduchinok as dotnet/fsharp#14985 is now available in JFCS, I was able to enable subsequent fields case quite easily.

Any thoughts on what to do next in this PR?

@auduchinok
Copy link
Member

@auduchinok as dotnet/fsharp#14985 is now available in JFCS, I was able to enable subsequent fields case quite easily.

Any thoughts on what to do next in this PR?

It would be really cool to:

  • auto popup completion on subsequent named args
  • remove all other items and only show unmatched fields in the completion popup

@nojaf nojaf force-pushed the named-union-case-fields-rule-experiment branch from 5d6e4a3 to c34ef08 Compare June 27, 2023 09:53
@nojaf
Copy link
Contributor Author

nojaf commented Jun 27, 2023 •
edited
Loading

Hi @auduchinok, I think I got the suggestions working.

remove all other items and only show unmatched fields in the completion popup

image

The types are still in there for some reason. Not sure why?

@nojaf nojaf marked this pull request as ready for review June 27, 2023 13:04
@nojaf
Copy link
Contributor Author

nojaf commented Jun 27, 2023

@auduchinok thanks for all the pointers earlier today.
I did still need to keep the RemoveWhen code in NamedUnionCaseFieldsPatRule.
There was still some stuff that was coming through.

@nojaf
Copy link
Contributor Author

nojaf commented Jun 27, 2023

I think the updates to the existing tests are valid if you consider Value in:
https://github.com/dotnet/fsharp/blob/124db19c11ac940ab2e3b33d50194b5c7e0ffc7e/src/FSharp.Core/prim-types.fs#L3867-L3869

auduchinok
auduchinok reviewed Jun 27, 2023
View reviewed changes
Copy link
Member

@auduchinok auduchinok left a comment

Choose a reason for hiding this comment

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

@nojaf nice work! I found a pair of interesting cases, could you look into them?

...arper.FSharp/src/FSharp.Psi.Features/src/CodeCompletion/Rules/NamedUnionCaseFieldsPatRule.fs Outdated Show resolved Hide resolved
...arper.FSharp/src/FSharp.Psi.Features/src/CodeCompletion/Rules/NamedUnionCaseFieldsPatRule.fs Outdated Show resolved Hide resolved
@nojaf
Copy link
Contributor Author

nojaf commented Jun 28, 2023

I think I got it.

auduchinok
auduchinok reviewed Jun 28, 2023
View reviewed changes
Copy link
Member

@auduchinok auduchinok left a comment

Choose a reason for hiding this comment

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

@nojaf All looks really nice! I think there's just one thing left.

ReSharper.FSharp/test/data/features/completion/filtered/NamedUnionCaseFieldsPat - 07.fs.gold Outdated Show resolved Hide resolved
...arper.FSharp/src/FSharp.Psi.Features/src/CodeCompletion/Rules/NamedUnionCaseFieldsPatRule.fs Outdated Show resolved Hide resolved
@auduchinok
Copy link
Member

auduchinok commented Jun 28, 2023 •
edited
Loading

@nojaf Could you also add a case in FSharpCodeCompletionTypingTest, similar to the record tests there, please? It will capture the autopopup logic.

auduchinok
auduchinok approved these changes Jun 28, 2023
View reviewed changes
Copy link
Member

@auduchinok auduchinok left a comment

Choose a reason for hiding this comment

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

This is really nice, thanks @nojaf!

@auduchinok auduchinok changed the title Named union case fields rule experiment Completion/rules: named union case fields Jun 28, 2023
@auduchinok auduchinok merged commit b5522c0 into JetBrains:net232 Jun 28, 2023
@auduchinok auduchinok changed the title Completion/rules: named union case fields Code completion: named union case fields rule Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@auduchinok auduchinok auduchinok approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nojaf @En3Tho @auduchinok