-
Notifications
You must be signed in to change notification settings - Fork 830
Better completion #3940
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 #3940
Conversation
saul
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Any chance we could add a test for this?
|
@dotnet-bot test Windows_NT Release_ci_part1 Build please |
|
It's ready. |
|
There is no ast until you close the paren. This is the same problem as with attributes completion - it suggests full type name until you close >] |
|
Yeah, just debugged this and noticed it 😄 . I suppose we can file that as a future problem to tackle. This looks good! |
|
I’m not sure we can solve this problem without improving the parser. And i’m not sure it could be improved :) |
| | AttributeApplication | ||
| | OpenDeclaration | ||
| /// completing pattern type (e.g. foo (x: |)) | ||
| | PatternType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be named ParameterTypeAnnotation or TypeAnnotation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like TypeAnnotation would be the better candidate of my two suggestions since it is also hit here:
let foo (s: string): ^There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is for xx: type at any place, not just for parameters. And this thing is called pattern I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think what you're referring to is the patterns that have type annotations, so PatternWithTypeAnnotation would be the name I'd think of here. That may have been what you meant, but when I read PatternType I thought it was referring to a completion context which constituted a pattern of any kind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe Pattern refers to the bound pattern (when deconstructing a DU, a tuple, a record, etc.) and Type to the type annotation.
let foo ((Some s): string option) = ...parameter can be a simple symbol but also can be a pattern.
|
Ooookey, it's ready. |
KevinRansom
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this.

This partially fixes #3937