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

discover CSX files as launch targets #1247

Merged
merged 3 commits into from
Mar 8, 2017
Merged

Conversation

filipw
Copy link
Contributor

@filipw filipw commented Feb 16, 2017

This PR enables CSX to be discovered as valid OmniSharp launch points.
Allows CSX intellisense / language services to be used without a "fake" project.json that's currently needed (for OmniSharp to light up).

screenshot 2017-02-15 23 46 16

@DustinCampbell
Copy link
Member

Is this the right experience? Should OmniSharp be launched on a particular .csx? Or should all csx files just be loaded by default?

@filipw
Copy link
Contributor Author

filipw commented Feb 17, 2017

good point, OmniSharp will just load all the CSX files starting from the entry directory, so we don't need to launch OmniSharp at a specific CSX file.
So maybe the best idea is to always include a root-level single "catch-all" CSX launch target?

I still think a CSX launch target (even if it's a root catch all one) needs to be there for OmniSharp to light up at all; in many cases (i.e. CSX files under folders containing sln/csproj/project.json) it wouldn't be needed at all, but in case where those are not there at all (i.e. folder is CSX only or CSX is outside of the slns/csproj/project.json) it's required

@filipw
Copy link
Contributor Author

filipw commented Feb 17, 2017

I pushed this change, let me know if it makes more sense now

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.

Sorry for the delay @filipw. I think this is fine for now. I want to revisit the whole launcher logic since it's pretty clunky. However, your change seems good with the current state of the world. 😄

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.

3 participants