Skip to content

External access #8046

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 21 commits into from
Feb 8, 2023
Merged

External access #8046

merged 21 commits into from
Feb 8, 2023

Conversation

@ryanbrandenburg
Copy link
Contributor

@ryanbrandenburg ryanbrandenburg commented Dec 22, 2022 •
edited
Loading

Summary of the changes

  • We're turning this dll into a true ExternalAccess dll.

Fixes: Part of #6945.

@ryanbrandenburg ryanbrandenburg requested review from a team as code owners December 22, 2022 22:40
davidwengier
davidwengier reviewed Dec 22, 2022
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.

I no understand

@ryanbrandenburg
Copy link
Contributor Author

ryanbrandenburg commented Jan 3, 2023

No merging this until at least Friday.

@davidwengier
Copy link
Member

davidwengier commented Jan 3, 2023

Can you update the description with what this is intended to do and how it helps? I've forgotten most of the context here.

Also, do we need to coordinate anything with O# given the rename of a DLL? Or do/did they just use the non-strong-named one?

@davidwengier
Copy link
Member

davidwengier commented Jan 4, 2023

No merging this until at least Friday.

We should probably wait for Allison's branch to merge in, too

davidwengier
davidwengier reviewed Jan 20, 2023
View reviewed changes
davidwengier
davidwengier approved these changes Feb 3, 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.

This is a nice and small EA project :)

Should add the public API analyzers before merging (unless you're going back to IVTs in the end?)

allisonchou
allisonchou approved these changes Feb 8, 2023
View reviewed changes
Copy link
Contributor

@allisonchou allisonchou left a comment

Choose a reason for hiding this comment

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

Nice! Per our earlier convo I don't think this should break anything, crossing fingers

...or/test/Microsoft.AspNetCore.Razor.OmniSharpPlugin.Test/DefaultProjectChangePublisherTest.cs
[InlineData(nameof(OmniSharpProjectChangeKind.DocumentAdded))]
[InlineData(nameof(OmniSharpProjectChangeKind.DocumentRemoved))]
[InlineData(nameof(OmniSharpProjectChangeKind.ProjectChanged))]
public async Task ProjectManager_Changed_EnqueuesPublish(string changeKindStr)
Copy link
Contributor

@allisonchou allisonchou Feb 8, 2023

Choose a reason for hiding this comment

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

I think the VS Code tests were actually disabled in CI a while ago, lol might be good to eventually verify they still work/pass 😅 (prob can be done in a different PR)

@ryanbrandenburg ryanbrandenburg merged commit 88aca56 into dotnet:main Feb 8, 2023
@allisonchou allisonchou mentioned this pull request Feb 23, 2023
Closed
@allisonchou allisonchou mentioned this pull request Mar 22, 2023
Merged
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

@allisonchou allisonchou allisonchou approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ryanbrandenburg @davidwengier @allisonchou