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

solution support, using fsac workspaces #480

Merged
merged 3 commits into from Sep 10, 2017

Conversation

Projects
None yet
4 participants
@enricosada
Collaborator

enricosada commented Aug 2, 2017

REQUIRE https://github.com/fsharp/FsAutoComplete master who add workspacePeek command to FSAC

idea is to:

  • at start, ask fsac with workspacePeek command what is interesting in that directory
  • fsac respond with list of parsed sln and list of fsprojs
  • based on config, is choosen: a sln or the directory or the user choose from list
  • solution explorer will show the full parsed solution graph, or the directory fsproj. these fsproj are not yet loaded (but will as lazy loading)

Modes using config FSharp.workspaceMode

  • sln (default) -> prefer sln, if any. Use the sln if only one exists, ask user with list if multiple exists. load the directory if noone
  • directory -> like before, wild west of fsprojs (no sln needed). but the search is done fsac side
  • ionideSearch-> previous behaviour, search of fsproj vscode side

additional configurations:

  • FSharp.workspaceModePeekDeepLevel: The deep level of directory hierarchy when searching for sln/projects. Default 2 ( so search up to ./src/SomeprojectDir/ )

New commands:

  • F#: Change Workspace or Solution will show the list to change sln or use the directory

Notes:

  • if list is shown (like at start), and is pressed esc, the ionideSearch is used. same in case of error of workspacePeek command

  • Added project loading state to F# Project Explorer during project command ( not yet loaded -> loading -> failed -> loaded)

  • Right click on failed loaded project in solution explorer -> see error info about loading (same printed to F# panel, but easier to discover)

  • Right click on successfully loaded project in solution explorer -> see info

Example ask:

sln

NOT IN THIS PR:

  • be strict? if a source file is not in the fsac workspace, dont do intellisense (project will be not parsed -> will not work -> squiggles)

@enricosada enricosada requested a review from Krzysztof-Cieslak Aug 2, 2017

@baronfel

This comment has been minimized.

Show comment
Hide comment
@baronfel

baronfel Aug 2, 2017

Contributor

Is is possible to jump-start this process when code is invoked?

Say on the command line I invoke code path/to/solution.sln, would it be possible to begin the solution load process with that file?

Contributor

baronfel commented Aug 2, 2017

Is is possible to jump-start this process when code is invoked?

Say on the command line I invoke code path/to/solution.sln, would it be possible to begin the solution load process with that file?

@enricosada

This comment has been minimized.

Show comment
Hide comment
@enricosada

enricosada Aug 2, 2017

Collaborator
Collaborator

enricosada commented Aug 2, 2017

@Krzysztof-Cieslak

This comment has been minimized.

Show comment
Hide comment
@Krzysztof-Cieslak

Krzysztof-Cieslak Aug 2, 2017

Member

under ask mode, user choose what to use

I'd want to use by default current mode, I like it, I believe that .sln files should be burned with fire. And while I like the PR I think using sln should be opt-in

Member

Krzysztof-Cieslak commented Aug 2, 2017

under ask mode, user choose what to use

I'd want to use by default current mode, I like it, I believe that .sln files should be burned with fire. And while I like the PR I think using sln should be opt-in

@Krzysztof-Cieslak

This comment has been minimized.

Show comment
Hide comment
@Krzysztof-Cieslak

Krzysztof-Cieslak Aug 2, 2017

Member

@baronfel:

Is is possible to jump-start this process when code is invoked? Say on the command line I invoke code path/to/solution.sln, would it be possible to begin the solution load process with that file?

Not possible, invoking code path/to/solution.sln will fire Code in "single-file" mode, with just the file opened

Member

Krzysztof-Cieslak commented Aug 2, 2017

@baronfel:

Is is possible to jump-start this process when code is invoked? Say on the command line I invoke code path/to/solution.sln, would it be possible to begin the solution load process with that file?

Not possible, invoking code path/to/solution.sln will fire Code in "single-file" mode, with just the file opened

@enricosada

This comment has been minimized.

Show comment
Hide comment
@enricosada

enricosada Aug 2, 2017

Collaborator
Collaborator

enricosada commented Aug 2, 2017

@enricosada

This comment has been minimized.

Show comment
Hide comment
@enricosada

enricosada Aug 5, 2017

Collaborator

A bit bumpy while loading alll projects...

sln_yet

Collaborator

enricosada commented Aug 5, 2017

A bit bumpy while loading alll projects...

sln_yet

@enricosada

This comment has been minimized.

Show comment
Hide comment
@enricosada

enricosada Aug 6, 2017

Collaborator
  • cleaned up initialization.
  • consolidate old and new mode
  • now is not bumpy, and display (not loaded -> loading -> loaded|failed)
  • added right click with info about why failure or status (useful later for target fw, etc)

sln_errors

Collaborator

enricosada commented Aug 6, 2017

  • cleaned up initialization.
  • consolidate old and new mode
  • now is not bumpy, and display (not loaded -> loading -> loaded|failed)
  • added right click with info about why failure or status (useful later for target fw, etc)

sln_errors

@enricosada

This comment has been minimized.

Show comment
Hide comment
@enricosada

enricosada Aug 7, 2017

Collaborator

Added cmd to change workspace or solution

sln_change

Collaborator

enricosada commented Aug 7, 2017

Added cmd to change workspace or solution

sln_change

@enricosada enricosada changed the title from [WIP] solution support, using fsac workspaces to solution support, using fsac workspaces Aug 8, 2017

@enricosada

This comment has been minimized.

Show comment
Hide comment
@enricosada

enricosada Aug 8, 2017

Collaborator

@Krzysztof-Cieslak this is done for me. i am satified atm with result and UX.
Open to feedback about UX and code

default is previous behaviour, so i hope can be published, and meanwhile tested by user changing the user setting, before change the default behaviour.

REQUIRE fsharp/FsAutoComplete#191 who add workspacePeek command to FSAC, that PR is done and in review atm

Collaborator

enricosada commented Aug 8, 2017

@Krzysztof-Cieslak this is done for me. i am satified atm with result and UX.
Open to feedback about UX and code

default is previous behaviour, so i hope can be published, and meanwhile tested by user changing the user setting, before change the default behaviour.

REQUIRE fsharp/FsAutoComplete#191 who add workspacePeek command to FSAC, that PR is done and in review atm

@Krzysztof-Cieslak

This comment has been minimized.

Show comment
Hide comment
@Krzysztof-Cieslak

Krzysztof-Cieslak Aug 8, 2017

Member

Another problem is that we're on FSAC's structuredSignature branch (which is branch out of cache branch) and we need to get it merged to master too... (or at least rebased after fsharp/FsAutoComplete#191 is merged)

Member

Krzysztof-Cieslak commented Aug 8, 2017

Another problem is that we're on FSAC's structuredSignature branch (which is branch out of cache branch) and we need to get it merged to master too... (or at least rebased after fsharp/FsAutoComplete#191 is merged)

@enricosada

This comment has been minimized.

Show comment
Hide comment
@enricosada

enricosada Aug 8, 2017

Collaborator

@Krzysztof-Cieslak yes seen that.
Ihmo we can wait that your fsharp/FsAutoComplete#186 is merged, and we are back on fsac parity

To just test early this PR, is ok to use my PR's fsac.suave.exe with let devMode = true (that's what i do anyway).
Because i didnt change how project are loaded, but just the loading sequence

I'll try to check why your PR tests fails this weekend

Collaborator

enricosada commented Aug 8, 2017

@Krzysztof-Cieslak yes seen that.
Ihmo we can wait that your fsharp/FsAutoComplete#186 is merged, and we are back on fsac parity

To just test early this PR, is ok to use my PR's fsac.suave.exe with let devMode = true (that's what i do anyway).
Because i didnt change how project are loaded, but just the loading sequence

I'll try to check why your PR tests fails this weekend

@@ -606,6 +641,7 @@
"workspaceContains:**/*.fs",
"workspaceContains:**/*.fsx",
"workspaceContains:**/*.fsproj",
"workspaceContains:**/*.sln",

This comment has been minimized.

@Krzysztof-Cieslak

Krzysztof-Cieslak Aug 8, 2017

Member

This will mean that Ionide is activated for C#-only projects, do we (and users) really want it?

@Krzysztof-Cieslak

Krzysztof-Cieslak Aug 8, 2017

Member

This will mean that Ionide is activated for C#-only projects, do we (and users) really want it?

Show outdated Hide outdated src/Components/SolutionExplorer.fs
Show outdated Hide outdated src/Components/SolutionExplorer.fs
Show outdated Hide outdated src/Core/Project.fs
let provider = createProvider emiter
Project.projectChanged.event.Invoke(fun proj ->
Project.workspaceChanged.event.Invoke(fun _ ->
emiter.fire (unbox ()) |> unbox)

This comment has been minimized.

@vasily-kirichenko

vasily-kirichenko Aug 8, 2017

Contributor

wow. pure magic :)

@vasily-kirichenko

vasily-kirichenko Aug 8, 2017

Contributor

wow. pure magic :)

This comment has been minimized.

@enricosada

enricosada Aug 9, 2017

Collaborator

yes, copy pasted it

@enricosada

enricosada Aug 9, 2017

Collaborator

yes, copy pasted it

[ "<b>Status:</b> failed to load"; ""
"<b>Error:</b>"
error ]
|> String.concat "<br />"

This comment has been minimized.

@vasily-kirichenko

vasily-kirichenko Aug 8, 2017

Contributor

Maybe just put <br /> at the end of each line?

@vasily-kirichenko

vasily-kirichenko Aug 8, 2017

Contributor

Maybe just put <br /> at the end of each line?

{ new TextDocumentContentProvider with
member this.provideTextDocumentContent (uri: Uri) =
match uri.path with
| "/projects/status" ->

This comment has been minimized.

@vasily-kirichenko

vasily-kirichenko Aug 8, 2017

Contributor

Is it always in lower case?

@vasily-kirichenko

vasily-kirichenko Aug 8, 2017

Contributor

Is it always in lower case?

This comment has been minimized.

@enricosada

enricosada Aug 9, 2017

Collaborator

yes, that TextDocumentContentProvider is something registered and internally used by ionide, so we create both the request urls and the handler

let projectStatusUri projectPath = vscode.Uri.parse(sprintf "fsharp-workspace://authority/projects/status?path=%s" (JS.encodeURIComponent(projectPath)))
@enricosada

enricosada Aug 9, 2017

Collaborator

yes, that TextDocumentContentProvider is something registered and internally used by ionide, so we create both the request urls and the handler

let projectStatusUri projectPath = vscode.Uri.parse(sprintf "fsharp-workspace://authority/projects/status?path=%s" (JS.encodeURIComponent(projectPath)))
Id: string
ConfigurationName: string
PlatformName: string
}

This comment has been minimized.

@vasily-kirichenko

vasily-kirichenko Aug 8, 2017

Contributor

Maybe use a namespace rec or module rec?

@vasily-kirichenko

vasily-kirichenko Aug 8, 2017

Contributor

Maybe use a namespace rec or module rec?

@Krzysztof-Cieslak

This comment has been minimized.

Show comment
Hide comment
@Krzysztof-Cieslak

Krzysztof-Cieslak Aug 15, 2017

Member

Due to recent changes to project explorer (lot of great quality of life changes) this will need rebasing, sorry for trouble :(

Member

Krzysztof-Cieslak commented Aug 15, 2017

Due to recent changes to project explorer (lot of great quality of life changes) this will need rebasing, sorry for trouble :(

@enricosada

This comment has been minimized.

Show comment
Hide comment
@enricosada

enricosada Aug 16, 2017

Collaborator

will do, np

Collaborator

enricosada commented Aug 16, 2017

will do, np

@enricosada

This comment has been minimized.

Show comment
Hide comment
@enricosada

enricosada Sep 1, 2017

Collaborator

rebased to fsharp/FsAutoComplete#200

Added more info about project, using new info returned by FSAC project response

image

Collaborator

enricosada commented Sep 1, 2017

rebased to fsharp/FsAutoComplete#200

Added more info about project, using new info returned by FSAC project response

image

@enricosada

This comment has been minimized.

Show comment
Hide comment
@enricosada

enricosada Sep 1, 2017

Collaborator

ok, i need just to tweak a bit the option for configuration now

Collaborator

enricosada commented Sep 1, 2017

ok, i need just to tweak a bit the option for configuration now

@enricosada

This comment has been minimized.

Show comment
Hide comment
@enricosada

enricosada Sep 2, 2017

Collaborator

Ok @Krzysztof-Cieslak , updated PR description.

pretty satified with this one. default are sane, old behaviour exists too if needed.

Let's wait ionide catch up FSAC master HEAD

Collaborator

enricosada commented Sep 2, 2017

Ok @Krzysztof-Cieslak , updated PR description.

pretty satified with this one. default are sane, old behaviour exists too if needed.

Let's wait ionide catch up FSAC master HEAD

@enricosada enricosada referenced this pull request Sep 4, 2017

Closed

Wishlist: generate SLN #523

@Krzysztof-Cieslak

This comment has been minimized.

Show comment
Hide comment
@Krzysztof-Cieslak

Krzysztof-Cieslak Sep 4, 2017

Member

Requires rebasing again, I guess you can also bump FSAC to include all recent fixes

Member

Krzysztof-Cieslak commented Sep 4, 2017

Requires rebasing again, I guess you can also bump FSAC to include all recent fixes

@Krzysztof-Cieslak

This comment has been minimized.

Show comment
Hide comment
@Krzysztof-Cieslak

Krzysztof-Cieslak Sep 5, 2017

Member

I'm bit worried about default settings:

  • as I've mentioned before I think old mode should be default one and users needs to opt-in for sln.
  • search level 2 seems not enough for some structures I've seen in real world
Member

Krzysztof-Cieslak commented Sep 5, 2017

I'm bit worried about default settings:

  • as I've mentioned before I think old mode should be default one and users needs to opt-in for sln.
  • search level 2 seems not enough for some structures I've seen in real world
@Krzysztof-Cieslak

This comment has been minimized.

Show comment
Hide comment
@Krzysztof-Cieslak

Krzysztof-Cieslak Sep 5, 2017

Member

What will happen if mode is "directory" and user will run "Change workspace or solution" command?

Member

Krzysztof-Cieslak commented Sep 5, 2017

What will happen if mode is "directory" and user will run "Change workspace or solution" command?

enricosada added some commits Aug 2, 2017

add sln support, enhance f# project explorer
Modes using config `FSharp.workspaceMode`

- `sln` (**default**) ->
    prefer sln, if any. Use the sln if only one exists, ask user with list if
    multiple exists. load the directory if noone
- `directory` ->
    like before, wild west of fsprojs (no sln needed). but the search is done
    fsac side
- `ionideSearch`->
    previous behaviour, search of fsproj vscode side

additional configurations:

- `FSharp.workspaceModePeekDeepLevel`:
    The deep level of directory hierarchy when searching for sln/projects.
    Default 2 ( so search up to `./src/SomeprojectDir/` )

New commands:

- `F#: Change Workspace or Solution`
    will show the list to change sln or use the directory

Notes:

- if list is shown (like at start), and is pressed esc, the `ionideSearch` is
  used. same in case of error of `workspacePeek` command
- Added project loading state to `F# Project Explorer` during `project` command
  ( not yet loaded ->  loading -> failed -> loaded)
- Right click on failed loaded project in solution explorer -> see error info
  about loading (same printed to F# panel, but easier to discover)
- Right click on successfully loaded project in solution explorer -> see info
dont `project` or `parse` if loading is already in progress.
also avoid useless `project`, if is already loaded

if project not in workspace, or loading done, guess the project like before
@enricosada

This comment has been minimized.

Show comment
Hide comment
@enricosada

enricosada Sep 5, 2017

Collaborator

as I've mentioned before I think old mode should be default one and users needs to opt-in for sln.

the sln is automatically loaded, if there only one.
The reason are:

  • if exists a sln, probably is what the use want to open in that repo.
  • If more than one sln => list with all sln + directory
  • If zero => like directory mode, directly load the project in the directory

Making directory the default, make harder to load the sln, and that's in lot of repo atm.
User can change default behaviour with a just setting
Ihmo is harmless to make that sln (is more a prefer sln if exists) the default, help existing repo and user.
And is the implicit default, can be changed later. Or an info box "Automatically loaded My.sln. this behaviour can disabled in config` if really is so bad

search level 2 seems not enough for some structures I've seen in real world

i sampled some proj in github, and seems enough. there is also a config for that, if use want to change.
But full free to use 3, if make more sense ( 2 = seach of proj like inside ./src/Project, 3 = seach of proj like inside ./src/Group/Project ).
Lot of code i see is like ./src/MyProject/MyProject.fsproj, same for tests
Also, if use press esc when shown the list, old behaviour (ionide search side) is used

What will happen if mode is "directory" and user will run "Change workspace or solution" command?

Nothing fancy. Another peek request, and the user see the list. If choose something, that is loaded. If use cancel (usual ESC or click in another part) nothing change.
it's a real choose from list, the default matter just for initial loading.

Collaborator

enricosada commented Sep 5, 2017

as I've mentioned before I think old mode should be default one and users needs to opt-in for sln.

the sln is automatically loaded, if there only one.
The reason are:

  • if exists a sln, probably is what the use want to open in that repo.
  • If more than one sln => list with all sln + directory
  • If zero => like directory mode, directly load the project in the directory

Making directory the default, make harder to load the sln, and that's in lot of repo atm.
User can change default behaviour with a just setting
Ihmo is harmless to make that sln (is more a prefer sln if exists) the default, help existing repo and user.
And is the implicit default, can be changed later. Or an info box "Automatically loaded My.sln. this behaviour can disabled in config` if really is so bad

search level 2 seems not enough for some structures I've seen in real world

i sampled some proj in github, and seems enough. there is also a config for that, if use want to change.
But full free to use 3, if make more sense ( 2 = seach of proj like inside ./src/Project, 3 = seach of proj like inside ./src/Group/Project ).
Lot of code i see is like ./src/MyProject/MyProject.fsproj, same for tests
Also, if use press esc when shown the list, old behaviour (ionide search side) is used

What will happen if mode is "directory" and user will run "Change workspace or solution" command?

Nothing fancy. Another peek request, and the user see the list. If choose something, that is loaded. If use cancel (usual ESC or click in another part) nothing change.
it's a real choose from list, the default matter just for initial loading.

@enricosada

This comment has been minimized.

Show comment
Hide comment
@enricosada

enricosada Sep 5, 2017

Collaborator

@Krzysztof-Cieslak so i rebase and squashed.

This PR already point to latest FSAC master HEAD with all the fixes, so if anyway want to try, is just usual run, no special FSAC needed

The last two commit are an enhancemnt ihmo:

  • wait to parse and project the visible files open at start, until all initial loading of projects is done.
  • after all the initial loading is done, reparse visible files.

This help because old behaviour try to guess the fsproj based on source file position. This is not enough lot of times, because may be in a relative path. Or for example a project my depends on others p2p ref, so is not enough.
waiting to load all projects give the best UX ihmo, help avoid the red squigless.
NOTE: waiting load of all project mean all project are done, good or bad.

Collaborator

enricosada commented Sep 5, 2017

@Krzysztof-Cieslak so i rebase and squashed.

This PR already point to latest FSAC master HEAD with all the fixes, so if anyway want to try, is just usual run, no special FSAC needed

The last two commit are an enhancemnt ihmo:

  • wait to parse and project the visible files open at start, until all initial loading of projects is done.
  • after all the initial loading is done, reparse visible files.

This help because old behaviour try to guess the fsproj based on source file position. This is not enough lot of times, because may be in a relative path. Or for example a project my depends on others p2p ref, so is not enough.
waiting to load all projects give the best UX ihmo, help avoid the red squigless.
NOTE: waiting load of all project mean all project are done, good or bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment