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

EditorFeatures.Text IVTs for MonoDevelop #31929

Merged
merged 1 commit into from Jan 2, 2019

Conversation

sandyarmstrong
Copy link
Member

Since #30868, ContentTypeNames
has moved to this assembly. Type forwarding doesn't seem to work for
IVT.

Since dotnet#30868, `ContentTypeNames`
has moved to this assembly. Type forwarding doesn't seem to work for
IVT.
@sandyarmstrong sandyarmstrong requested a review from a team as a code owner December 19, 2018 00:58
@sandyarmstrong
Copy link
Member Author

@Therzok

@sharwell
Copy link
Member

@sandyarmstrong Can you confirm that prior to #30868, you were already referencing ContentTypeNames at its original location via IVTs?

@Therzok
Copy link
Contributor

Therzok commented Dec 19, 2018

Yup: http://source.monodevelop.com/#MonoDevelop.CSharpBinding/MonoDevelop.Ide.Completion.Presentation/RoslynCompletionPresenter.cs,30

Clicking ContentTypeNames redirects to a (broken) roslyn page, but it's coming from roslyn EditorFeatures!

@sandyarmstrong
Copy link
Member Author

@sharwell As Marius says, yes, it's already being used. Not hard to work around, but it would be great to get this change in 2.11 so we can avoid any other unexpected surprises.

Copy link
Member

@sharwell sharwell left a comment

Choose a reason for hiding this comment

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

Approving on the basis that this is a grandfathered IVT (new IVTs aren't being allowed at this time). The original IVTs (which remain in place) are here:

<InternalsVisibleTo Include="MonoDevelop.Ide" Key="$(MonoDevelopKey)" />
<InternalsVisibleTo Include="MonoDevelop.Refactoring" Key="$(MonoDevelopKey)" />
<InternalsVisibleTo Include="MonoDevelop.CSharpBinding" Key="$(MonoDevelopKey)" />
<InternalsVisibleTo Include="MonoDevelop.VBNetBinding" Key="$(MonoDevelopKey)" />
<InternalsVisibleTo Include="MonoDevelop.Ide.Tests" Key="$(MonoDevelopKey)" />
<InternalsVisibleTo Include="MonoDevelop.Refactoring.Tests" Key="$(MonoDevelopKey)" />
<InternalsVisibleTo Include="MonoDevelop.CSharpBinding.Tests" Key="$(MonoDevelopKey)" />
<InternalsVisibleTo Include="MonoDevelop.VBNetBinding.Tests" Key="$(MonoDevelopKey)" />
<InternalsVisibleTo Include="Roslyn.InteractiveWindow.UnitTests" Key="$(MonoDevelopKey)" />

@jasonmalinowski
Copy link
Member

Do we not have a public constant anywhere you can use for this?

@sandyarmstrong
Copy link
Member Author

@jasonmalinowski no, I don't see any other constant for "Roslyn Languages".

There are like 20 IVTs for this assembly for VSwin so I don't see why there are concerns about doing the same for VSmac?

@jasonmalinowski
Copy link
Member

@sandyarmstrong We're taking a new battle against IVTs; I'm had thought we had exposed these somewhere and wanted to make sure we did that before opening up more IVT.

Given that those constants won't change, should we just have you hard code them? 😄

Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

Approving since I agree with @sharwell this isn't really making things worse, and that assembly is rarely (if ever) changed at this point.

@vatsalyaagrawal vatsalyaagrawal added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Dec 20, 2018
@vatsalyaagrawal vatsalyaagrawal added this to InQueue in IDE: CommunityPR via automation Dec 20, 2018
@jaredpar jaredpar closed this Dec 28, 2018
@jaredpar jaredpar reopened this Dec 28, 2018
@jaredpar
Copy link
Member

Retriggering on latest.

@sandyarmstrong
Copy link
Member Author

@jaredpar Any idea about roslyn-integration-CI test failures?

@sandyarmstrong
Copy link
Member Author

@jaredpar looks like I'm not the only PR with roslyn-integration-CI failures (#31993, which merged recently, had the same errors).

@jaredpar
Copy link
Member

@sandyarmstrong the roslyn-integration-CI leg is not required at this time (largely due to relability issues). If that one fails it's typically okay. The @dotnet/roslyn-ide team can push back in cases where they think the failure is real.

@jinujoseph jinujoseph moved this from InQueue to LGTM in IDE: CommunityPR Jan 2, 2019
@jinujoseph jinujoseph added this to the 16.0.P2 milestone Jan 2, 2019
@sharwell sharwell merged commit a93de4b into dotnet:master Jan 2, 2019
IDE: CommunityPR automation moved this from LGTM to Completed Jan 2, 2019
@sandyarmstrong
Copy link
Member Author

sandyarmstrong commented Jan 2, 2019

@sharwell thanks! Is it possible to get this into 2.11? If so, do I need to submit another PR to another branch (I'm guessing dev16.0.x)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved to merge Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
IDE: CommunityPR
  
Completed-2018
Development

Successfully merging this pull request may close these issues.

None yet

7 participants