[WIP] GotoDefinition Generates Metadata Signature File For External Symbols #2658
Conversation
- workspace still isn't updating properly
cloudRoutine
force-pushed
the
signature-metadata-nav
branch
from
0db1108 to
8011f05
Compare
|
A lot of code looks like reimplementation of layout + niceprint. It would be great to use nicerpint here, after some enhancements, I guess. |
| ( checkerProvider: FSharpCheckerProvider | ||
| , projectInfoManager: ProjectInfoManager | ||
| , signatureMetadataService : NavigateToSignatureMetadataService | ||
| , [<ImportMany>]_presenters: IEnumerable<INavigableItemsPresenter> |
There was a problem hiding this comment.
Haskell style detected.
There was a problem hiding this comment.
We should keep any style which doesn't look unkept and look like some work was put to layout the code for better reading.
Trying to harmonize the codebase at large to a single style doesn't seem bring as much as just making sure code has had efforts to make readable.
I prefer this (by far) versus all on single or two lines.
There was a problem hiding this comment.
most of the time i end up writing this way because i'm commenting and switching out services in the importing constructor, trying to figure out which one will actually do the right thing. it's more practical than stylistic
There was a problem hiding this comment.
We should keep any style which doesn't look unkept and look like some work was put to layout the code for better reading. Trying to harmonize the codebase at large to a single style doesn't seem bring as much as just making sure code has had efforts to make readable.
The contribution guidelines are clear - use the styles that are already inn use in the compiler. Please don't add new stylings
| open Microsoft.VisualStudio.TextManager.Interop | ||
| open Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem | ||
|
|
||
| [<Export>][<Shared>] |
There was a problem hiding this comment.
I believe Shared is by default, you can remove it.
There was a problem hiding this comment.
Please use [<Export; Shared>] if there are multipe attributes
| type internal NavigateToSignatureMetadataService [<ImportingConstructor>] | ||
| ( checkerProvider: FSharpCheckerProvider | ||
| , projectInfoManager: ProjectInfoManager | ||
| , workspace : VisualStudioWorkspaceImpl |
| | WorkspaceChangeKind.SolutionReloaded | ||
| | WorkspaceChangeKind.SolutionRemoved -> | ||
| currentWindow | ||
| |> Option.bind Option.ofNull |
There was a problem hiding this comment.
I think we should use Option.ofObj form FSharp.Core.
| match trimmedXml.[0] with | ||
| | '<' -> String.Join ("", "<root>", xml, "</root>") | ||
| | _ -> let escapedXml = SecurityElement.Escape xml | ||
| String.Join ("", "<summary>", escapedXml, "</summary>") |
There was a problem hiding this comment.
Could you indent branches? It's hard to follow. Maybe an active pattern for String.IsNullOrEmpty would work better here.
There was a problem hiding this comment.
Why not just "<summary>" + escapedXml + "</summary>" or sprintf "<summary>%s</summary>" escapedXml?
There was a problem hiding this comment.
@vasily-kirichenko I've seen this style of indentation when first branch is simple, and the code will need further cases added, I think it makes sense.
See https://github.com/smoothdeveloper/visualfsharp/blob/a3a3fb552d27ad8ba5cab6497a033ebf19655025/src/fsharp/NicePrint.fs#L1965-L2011 and try to indent the stuff, as you'll add cases, you'll need to go deeper on indentation and it will look really odd.
There was a problem hiding this comment.
@vasily-kirichenko I've seen this style of indentation when first branch is simple, and the code will need further cases added, I think it makes sense.
@smoothdeveloper Here @vasily-kirichenko's is correct that it is normal to indent branches for this kind of code
|
|
||
| | TypedAstPatterns.UnionCase uc -> | ||
| match uc.ReturnType with | ||
| | TypedAstPatterns.TypeWithDefinition entity -> |
There was a problem hiding this comment.
Inline this match right to | TypedAstPatterns.UnionCase uc ?
| OpenDeclarationGetter.updateOpenDeclsWithSymbolPrefix | ||
| [|"System"; "IO"|] (range (1, 1) (2, 2)) decls | ||
|
|
||
| CollectionAssert.AreEqual ([true; true], updatedDecls |> List.map (fun decl -> decl.IsUsed)) |
There was a problem hiding this comment.
Use defined above Collection.assertEqual or delete that and similar helpers.
| && not func.IsPropertySetterMethod | ||
| && not excluded | ||
| && not (isOperator func.DisplayName) -> Some() |
There was a problem hiding this comment.
@vasily-kirichenko here also, haskell police :)
| name.StartsWith "( " && name.EndsWith " )" && name.Length > 4 | ||
| && name.Substring (2, name.Length - 4) | ||
| |> String.forall (fun c -> c <> ' ' && not (Char.IsLetter c)) |
There was a problem hiding this comment.
suggestions:
- maybe this whole expression can be better laid out?
- also could you have comment on stuff dealing with substring as those are really annoying to reverse engineer / run in the debugger in my head.
proposal:
let isOperator (name: string) =
let inline notSpaceOrLetter c = c <> ' ' && not (Char.IsLetter c)
name.StartsWith "( "
&& name.EndsWith " )"
&& name.Length > 4
// check something something
&& (name.Substring (2, name.Length - 4) |> String.forall notSpaceOrLetter) 
|
@jasonmalinowski or @dpoeschl - can one of you take a look at this? |
| [<Guid(FSharpCommonConstants.packageGuidString)>] | ||
| [<ProvideLanguageService | ||
| ( languageService = typeof<FSharpLanguageService> | ||
| , strLanguageName = FSharpCommonConstants.FSharpLanguageName |
There was a problem hiding this comment.
Please no commas at start of line. Please stick to the coding conventions already in use in the codebase
|
If possible I'd much rather have |
Yes it would be good to have it generally available. I think we should probably just put the code you already have behind the SourceCodeServices boundary and accept the duplication - as long as you and @vasily-kirichenko feel the code is up to scratch and can be progressed. It will get tested quite thoroughly quite quickly by actual users (it's a very visible feature) so if there are deficiencies they will become apparent pretty quickly. |
|
@KevinRansom I think we should just merge it as is. Nobody is gonna make it "right". |
|
Yes - @cloudRoutine I'll reopen this if that's ok. I agree we need to get this into shape in some form (and it might be me who needs to do it since I'm responsible for the awfulness of the NicePrint.fs code) |
|
@dsyme sure np. I closed it since I'm gonna take a hiatus from working on VFT, it's outdated, full of conflicts, and I figured no one else would work on it. |
|
@cloudRoutine I will take over. |
|
@cartermp let's fix this. I need some advice as soon as I'm into it enough |
Yes, update to latest master. Submit a new PR if needed (though @cloudRoutine will likely give you push access to his repo if you wish) |
I think that will be hard to fix. There is no project context available within which to do this checking |
|
I certainly won't block on this:
The benefit of the other features definitely outweighs this issue. |
|
@dsyme Can't we reference all dll's of the project we are currently in? |
The F# Language Service needs a "project" for the .fsi file to live in. It is this project that must have the references We do already manufacture a "single file project" for the standalone .fsi file, and currently the list of referenced assemblies will be basically empty for that project (a default set of .NET Framework references are assumed). We could in theory add the references from the referencing project to this single-file project. However we don't do anything like that just yet, and I'm not sure it's that easy to plumb the information across. If it turns out to be easy then we should do it, but I think it can be separate as it's very different work to the current PR |
In Roslyn there is a |
|
My suggestion is to reference at least the assembly where the meta data is from. |
@realvictorprm You are totally right - we should either do this or not show any errors for these generated files. I'm really just saynig that we can tolerate the errors being shown initially and deal with it in a separate PR |
|
Is this still a WIP, or is it something that should be closed? Thanks Kevin |
|
Closing very old PR, I think maser will have diverged too far |
Only one readonly window will open to show signature metadata, I tried a bunch of different ways to make it a preview/provisional texteditor, but nothing panned out.