Skip to content

Fix completion inside last part of long ident#3711

Merged
KevinRansom merged 12 commits into
dotnet:masterfrom
vasily-kirichenko:fix-completion-inside-last-part-of-long-ident
Oct 14, 2017
Merged

Fix completion inside last part of long ident#3711
KevinRansom merged 12 commits into
dotnet:masterfrom
vasily-kirichenko:fix-completion-inside-last-part-of-long-ident

Conversation

@vasily-kirichenko

@vasily-kirichenko vasily-kirichenko commented Oct 8, 2017

Copy link
Copy Markdown
Contributor

This fixes #3709

Was:

image

Now:

image

@auduchinok

auduchinok commented Oct 8, 2017

Copy link
Copy Markdown
Member

Maybe we should move QuickParse.fs to FCS? It is VS-agnostic and can be used in all tools.

@vasily-kirichenko

Copy link
Copy Markdown
Contributor Author

@auduchinok done. Also I think parsing long idents should be hidden behind FCS API entirely. Stuff like plids, residue and lastDotPos should be removed, only line str and pos should be passed. It would simplify the API a lot, but I think it should be done in a separate PR.

@auduchinok

Copy link
Copy Markdown
Member

@vasily-kirichenko thank you!
I absolutely agree about simplifying these APIs. I've been thinking about adding a type just like you did in this PR.

@vasily-kirichenko

Copy link
Copy Markdown
Contributor Author

TBH, I didn't think much about that type, so it's not that elegant. The whole API needs a serious rethinking and simplifying based on real usage scenarios in Ionide, Rider and VS.

@vasily-kirichenko

Copy link
Copy Markdown
Contributor Author

It's ready

let UnusedOpensAnalyzerInitialDelay = 0 (* 2000 *) (* milliseconds *)
let SimplifyNameInitialDelay = 2000 (* milliseconds *)
let SimplifyNameEachItemDelay = 5 (* milliseconds *)
let SimplifyNameEachItemDelay = 0 (* milliseconds *)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

part of other PR?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes.

@dsyme dsyme left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Requesting a few minor changes

Comment thread src/fsharp/vs/QuickParse.fsi Outdated
module internal QuickParse =
#endif
val MagicalAdjustmentConstant : int
val CorrectIdentifierToken : s: string -> tokenTag: int -> int

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor things

  • Could you give better argument names please, e.g. tokenText not s?
  • Also in the implementation.
  • Also a /// comment on each public thing please
  • Add a blank line between before each /// comment section

Comment thread src/fsharp/vs/service.fsi Outdated
/// </param>
/// <param name="userOpName">An optional string used for tracing compiler operations associated with this request.</param>
member GetDeclarationListInfo : ParsedFileResultsOpt:FSharpParseFileResults option * line: int * colAtEndOfPartialName: int * lineText:string * qualifyingNames: string list * partialName: string * getAllSymbols: (unit -> AssemblySymbol list) * ?hasTextChangedSinceLastTypecheck: (obj * range -> bool) * ?userOpName: string -> Async<FSharpDeclarationListInfo>
member GetDeclarationListInfo : ParsedFileResultsOpt:FSharpParseFileResults option * line: int * colAtEndOfPartialName: int * lineText:string * partialName: PartialLongName * getAllSymbols: (unit -> AssemblySymbol list) * ?hasTextChangedSinceLastTypecheck: (obj * range -> bool) * ?userOpName: string -> Async<FSharpDeclarationListInfo>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • Please fix up the param documentation so the parameter list match. Also for GetDeclarationListSymbols

  • Please add a sentence in the /// comment text for partialName the saying that the QuickParse module can be used to get a partialName value

@vasily-kirichenko

Copy link
Copy Markdown
Contributor Author

All done, waiting until it's green.

@vasily-kirichenko

Copy link
Copy Markdown
Contributor Author

ready

@cartermp cartermp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Works well for me 👍

@dsyme dsyme left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

great job

@KevinRansom KevinRansom merged commit 860c812 into dotnet:master Oct 14, 2017
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
* make SimplifyNameEachItemDelay zero

* QuickParse.GetPartialLongNameEx returns pos of the last dot in long ident which is used father in GetDeclaredItems to determine the RHL type

* refactoring

* fix QuickParse

* move QuickParse to FSharp.Compiler.Private project

* add QuickParse to all projects which need it

* fix QuickParse

* add a test

* fix QuickParse

* xml docs

* embed colAtEndOfNamesAndResidue into PartialLongName

* revert unrelated change
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.

Wrong completion items for location

6 participants