Skip to content
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

Adding folders with Miscellaneous Cs files as Launch Targets #2471

Merged
merged 3 commits into from
Aug 28, 2018

Conversation

akshita31
Copy link
Contributor

@akshita31 akshita31 commented Aug 22, 2018

Fixes: #47

With the effort to support Miscellaneous files in O#(OmniSharp/omnisharp-roslyn#1252), modifying the LaunchTargets to add a folder with Misc files, if it found a cs file with no sln or csproj files

@codecov
Copy link

codecov bot commented Aug 22, 2018

Codecov Report

Merging #2471 into master will increase coverage by 0.41%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2471      +/-   ##
==========================================
+ Coverage      64%   64.41%   +0.41%     
==========================================
  Files          90       90              
  Lines        4114     4120       +6     
  Branches      590      592       +2     
==========================================
+ Hits         2633     2654      +21     
+ Misses       1312     1291      -21     
- Partials      169      175       +6
Flag Coverage Δ
#integration 53.31% <71.42%> (+0.19%) ⬆️
#unit 84.52% <ø> (ø) ⬆️
Impacted Files Coverage Δ
src/omnisharp/launcher.ts 53.73% <71.42%> (+2.16%) ⬆️
src/features/codeLensProvider.ts 46.6% <0%> (-1.95%) ⬇️
src/omnisharp/server.ts 72.43% <0%> (+0.7%) ⬆️
src/features/documentation.ts 38.09% <0%> (+2.38%) ⬆️
src/omnisharp/requestQueue.ts 89.39% <0%> (+4.54%) ⬆️
src/features/completionItemProvider.ts 77.77% <0%> (+17.46%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2e2177...4bd398c. Read the comment docs.

@colombod
Copy link
Member

looks good to me

Copy link
Member

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

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

I doubt that customers would understand when they would want select a launch target for "Miscellaneous" and what effect it would have. I'm also suspicious of the "CSX" and "Cake" launch targets. Why do I need three launch targets that do the same thing? (Note that the LaunchTarget.kind field is never used anywhere.)

Should we just consolidate all of these into a single launch target for the root folder?

cc @filipw about the CSX launch target

@@ -170,6 +177,16 @@ function resourcesToLaunchTargets(resources: vscode.Uri[]): LaunchTarget[] {
kind: LaunchTargetKind.Cake
});
}

if (hasCs && !hasSlnFile && !hasCsProjFiles && !hasProjectJson && !hasProjectJsonAtRoot) {
Copy link
Member

Choose a reason for hiding this comment

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

Having a separate launch target called "Miscellaneous" seems a little weird to me from a customer perspective. Wouldn't it be better if we just re-purpose the launch target here: https://github.com/OmniSharp/omnisharp-vscode/pull/2471/files#diff-0f649e373df5c5e30b19fa598a56b55bR149? If there are any C# files found but none of those conditions are met, we still want to be able to launch on the root folder.

Copy link

Choose a reason for hiding this comment

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

+1,

Copy link

@rchande rchande left a comment

Choose a reason for hiding this comment

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

Thanks!

@akshita31
Copy link
Contributor Author

@DustinCampbell I have removed the Misc file launch target and using the root folder as the launch target now. Does that seem right ?

@DustinCampbell
Copy link
Member

Yes, that seems right to me. I added @filipw as a reviewer to see if he has any concerns.

@akshita31 akshita31 merged commit 250f028 into dotnet:master Aug 28, 2018
@akshita31 akshita31 deleted the launch_config branch August 28, 2018 20:40
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.

4 participants