Skip to content

Conversation

@vasily-kirichenko
Copy link
Contributor

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

This fixes #3709

Was:

image

Now:

image

@auduchinok
Copy link
Member

auduchinok commented Oct 8, 2017

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

@vasily-kirichenko
Copy link
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
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
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
Contributor Author

It's ready

let UnusedOpensAnalyzerInitialDelay = 0 (* 2000 *) (* milliseconds *)
let SimplifyNameInitialDelay = 2000 (* milliseconds *)
let SimplifyNameEachItemDelay = 5 (* milliseconds *)
let SimplifyNameEachItemDelay = 0 (* milliseconds *)
Copy link
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
Contributor Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Contributor

@dsyme dsyme left a comment

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

module internal QuickParse =
#endif
val MagicalAdjustmentConstant : int
val CorrectIdentifierToken : s: string -> tokenTag: int -> int
Copy link
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

/// </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
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
Contributor Author

All done, waiting until it's green.

@vasily-kirichenko
Copy link
Contributor Author

ready

Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Works well for me 👍

Copy link
Contributor

@dsyme dsyme left a comment

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