Skip to content

Remove Omnisharp logic from main branch #9027

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Jul 27, 2023
Merged

Remove Omnisharp logic from main branch #9027

merged 17 commits into from
Jul 27, 2023

Conversation

@allisonchou
Copy link
Contributor

@allisonchou allisonchou commented Jul 26, 2023 •
edited
Loading

Summary of the changes

Removes the following projects from the main branch:

Made big modifications in Microsoft.AspNetCore.Razor.VSCode.Extension as well so that it basically only contains our TM grammar.

All the removed logic still resides in the release/omnisharp-vscode branch. This just removes the logic from the main branch since we no longer ship O# directly from it.

@allisonchou allisonchou requested a review from a team as a code owner July 26, 2023 01:47
@allisonchou allisonchou changed the title Remove Omnisharp logic Remove Omnisharp logic from main branch Jul 26, 2023
@davidwengier
Copy link
Member

davidwengier commented Jul 26, 2023 •
edited
Loading

I'm unsure about deleting this one because I think it partially tests our TM grammar

I think "partially" can be removed from that sentence. I'm not sure it does anything else 😛

without it we have zero Typescript and VS Code dependencies which is nice

This is very very very nice, and I award you 1000 points.

If we can move our grammar over to vscode-csharp soon then this would be a non-issue

I'll go further than "if" here: With the removal of TypeScript/node/etc. from our repo we are essentially blocked on making changes to our Textmate grammar. Granted that doesn't happen often, but wanted to be super clear about it. Sorry, now that I've looked at the code, I think I've misunderstood and the project is still here to build it, right? If so, and we still need npmproj etc., I think we might be better off keeping the tests too, until they can all move together.

@ryzngard
Copy link
Contributor

ryzngard commented Jul 26, 2023

Is the TextMate grammar used by VS? Could we somehow write an integration test for it?

@davidwengier
Copy link
Member

davidwengier commented Jul 26, 2023

Yes, our textmate grammar is used by VS. We defintely could write an integration test for it, though diagnosing issues would be hard. Ultimately it should live in the vscode-csharp repo, and we should just pull the built output from there, like we pull the Html and JavaScript grammars from external source now.

davidwengier
davidwengier reviewed Jul 26, 2023
View reviewed changes
...rosoft.AspNetCore.Razor.VSCode.Extension/Microsoft.AspNetCore.Razor.VSCode.Extension.npmproj
<TargetFramework>netstandard2.0</TargetFramework>
<IsPackable>false</IsPackable>
</PropertyGroup>

Copy link
Member

@davidwengier davidwengier Jul 26, 2023

Choose a reason for hiding this comment

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

I commented on the PR itself, but just so its not missed, if we still have projects that need node etc. then I think we are better off keeping the TextMate tests, until they can all move together

Copy link
Contributor Author

@allisonchou allisonchou Jul 26, 2023

Choose a reason for hiding this comment

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

Thanks for the feedback. I've added back the project for now and will look into moving all the grammar code in a follow up PR.

davidwengier
davidwengier approved these changes Jul 26, 2023
View reviewed changes
Copy link
Member

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

Love it!! REMOVE ALL THE CODE 😁

@allisonchou allisonchou merged commit b3c1f61 into main Jul 27, 2023
@allisonchou allisonchou deleted the dev/allichou/RemoveOmnisharp branch July 27, 2023 17:58
@ghost ghost added this to the Next milestone Jul 27, 2023
@allisonchou allisonchou mentioned this pull request Jul 31, 2023
Closed
333fred added a commit to 333fred/razor that referenced this pull request Aug 23, 2023
@333fred
Merge remote-tracking branch 'upstream/main' into sourcetext
527044e 
* upstream/main: (188 commits)
  Rename CSHTML file reference. (dotnet#8969)
  Remove Omnisharp logic from main branch (dotnet#9027)
  Update dependencies from https://github.com/dotnet/arcade build 20230726.1
  Fixes CVE-2023-33127 and CVE-2023-33170 (dotnet#9032)
  Remove Async Keyword For Generate Async Method Code Action (dotnet#9030)
  Remove dispatcher from DocumentVersionCache (dotnet#9026)
  Restore perf work. (dotnet#8995)
  Implement priority trigger support
  Change implementations and references
  Rename ProjectSnapshotChangeTrigger and convert to interface
  Updates after merge
  Fix nullability
  Use pattern matching
  Convert to record struct
  Move CloseTextTagOnAutoInsertProvider to FindToken (dotnet#9025)
  Move GenerateMethodCodeActionProvider to FindToken (dotnet#8988)
  Add comment describing when ProjectRazorJson.Version should be incremented
  Some more violations, after the merge
  Remove TryResolveDocument method
  [Infra] 17.8 P1 snap PRs (dotnet#9021)
  ...
@Cosifne Cosifne removed this from the Next milestone Sep 25, 2023
@Cosifne Cosifne added this to the 17.8 P3 milestone Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidwengier davidwengier davidwengier approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

17.8

Development

Successfully merging this pull request may close these issues.

5 participants

@allisonchou @davidwengier @ryzngard @Cosifne