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

Remove legacy completion #38569

Merged
merged 18 commits into from Sep 19, 2019

Conversation

@ivanbasov
Copy link
Contributor

commented Sep 7, 2019

No description provided.

@ivanbasov ivanbasov added this to the 16.4 milestone Sep 7, 2019
@ivanbasov ivanbasov requested review from genlu, sharwell and CyrusNajmabadi Sep 7, 2019
@ivanbasov ivanbasov requested review from dotnet/roslyn-ide as code owners Sep 7, 2019
@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2019

have not actually reviewed this. I presume it's just almost entirely the removal of the old code. so instead of looking at all that, was there anything more 'interesting' than that? if so, can you comment any areas that need particular attention? #Resolved

@jinujoseph

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2019

cc @KirillOsenkov for info and review
cc @sandyarmstrong #Resolved

@jasonmalinowski

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

@ivanbasov Can we target this at master? I recognize it'll create some merge conflicts into master-vs-deps, but does that make the code easier to maintain in master in the mean time? #Resolved

@ivanbasov

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

@ivanbasov Can we target this at master? I recognize it'll create some merge conflicts into master-vs-deps, but does that make the code easier to maintain in master in the mean time?

Thank you, @jasonmalinowski ! Good idea! Let us meet merge conflict once, rather than supporting 2 branches for a longer time. Will rebase into master. #Resolved

@ivanbasov

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

have not actually reviewed this. I presume it's just almost entirely the removal of the old code. so instead of looking at all that, was there anything more 'interesting' than that? if so, can you comment any areas that need particular attention?

Thank you, @CyrusNajmabadi ! I hope it is just a technical change: remove tests and remove the old implementation. I am not going to add anything interesting here. #Resolved

@ivanbasov ivanbasov changed the title Remove legacy completion WIP: Remove legacy completion Sep 9, 2019
@ivanbasov ivanbasov changed the base branch from master-vs-deps to master Sep 9, 2019
@ivanbasov ivanbasov force-pushed the ivanbasov:removelegacy branch from 225bd4b to 6632e02 Sep 9, 2019
Copy link
Member

left a comment

Primary concern is there are some tests that didn't get switched back to Facts, and thus they won't run. I know @sharwell was discussing a bit getting some of the xunit analyzers running on VB as well....this might be a really good reason to do so since it'd be hugely easy to make more mistakes like that.

ivanbasov and others added 3 commits Sep 10, 2019
Co-Authored-By: Jason Malinowski <jason@jason-m.com>
Co-Authored-By: Jason Malinowski <jason@jason-m.com>
Co-Authored-By: Jason Malinowski <jason@jason-m.com>
@sharwell

This comment was marked as resolved.

Copy link
Member

commented Sep 10, 2019

@jasonmalinowski The WPF attribute errors you mentioned apply to VB code, which is not currently supported by the xUnit analyzers. Once the analyzers are updated to support VB we can catch any remaining ones, but until then code review is the tool we have. :) #Resolved

@jasonmalinowski

This comment was marked as resolved.

Copy link
Member

commented Sep 10, 2019

@sharwell: the ones that matter here at least ("theories shouldn't have parameters") are already symbol analyzers, so updating the xunit analyzers to support them would be less than an hour of work, so wondering if we should just do it since that's perhaps easier than code review. #Resolved

@sharwell

This comment was marked as resolved.

Copy link
Member

commented Sep 10, 2019

@jasonmalinowski I'll look into getting them fixed and tests added #Resolved

@ivanbasov

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2019

Feedback addressed. @jasonmalinowski and @sharwell please review/unblock #Resolved

@jasonmalinowski jasonmalinowski dismissed their stale review Sep 13, 2019

Unblocking review as I won't have time to re-review.

@ivanbasov ivanbasov changed the title WIP: Remove legacy completion Remove legacy completion Sep 13, 2019
ivanbasov added 2 commits Sep 13, 2019
@@ -304,12 +304,6 @@ private void StartRemoteIntegrationService(DTE dte)
{
dte.ExecuteCommand(WellKnownCommandNames.Test_IntegrationTestService_Start);
}

if (AsyncCompletionCondition.Instance.ShouldSkip
&& dte.Commands.Item(WellKnownCommandNames.Test_IntegrationTestService_DisableAsyncCompletion).IsAvailable)

This comment has been minimized.

Copy link
@sharwell

sharwell Sep 16, 2019

Member

This command needs to be removed from WellKnownCommandNames and from the VSCT. #Resolved

This comment has been minimized.

Copy link
@sharwell

sharwell Sep 18, 2019

Member

📝 This is still problematic. Here are the locations I know need to be updated. There may be additional references that become obvious when you try to remove them.

<IDSymbol name="cmdidDisableAsyncCompletion" value="0x5203" />

<Button guid="guidTestWindowCmdSet" id="cmdidDisableAsyncCompletion" priority="0x0030" type="Button">
<Parent guid="guidTestWindowCmdSet" id="grpidIntegrationTestService" />
<Strings>
<ButtonText>Disable Async Completion</ButtonText>
<CommandName>IntegrationTestService.DisableAsyncCompletion</CommandName>
<CanonicalName>IntegrationTestService.DisableAsyncCompletion</CanonicalName>
<LocCanonicalName>IntegrationTestService.DisableAsyncCompletion</LocCanonicalName>
</Strings>
</Button>

public const int cmdidDisableAsyncCompletion = 0x5203;

var disableAsyncCompletionMenuCmdId = new CommandID(guidTestWindowCmdSet, cmdidDisableAsyncCompletion);
_disableAsyncCompletionMenuCmd = new MenuCommand(DisableAsyncCompletionCallback, disableAsyncCompletionMenuCmdId)
{
Enabled = true,
Visible = false,
};
menuCommandService.AddCommand(_disableAsyncCompletionMenuCmd);

private readonly MenuCommand _disableAsyncCompletionMenuCmd;

private void DisableAsyncCompletionCallback(object sender, EventArgs e)
{
if (!_disableAsyncCompletionMenuCmd.Enabled)
{
return;
}
var componentModel = ServiceProvider.GetService<SComponentModel, IComponentModel>();
var featureServiceFactory = componentModel.GetService<IFeatureServiceFactory>();
featureServiceFactory.GlobalFeatureService.Disable(PredefinedEditorFeatureNames.AsyncCompletion, EmptyFeatureController.Instance);
_disableAsyncCompletionMenuCmd.Enabled = false;
}
#Resolved

@ivanbasov ivanbasov merged commit cf78aee into dotnet:master Sep 19, 2019
18 checks passed
18 checks passed
WIP Ready for review
Details
license/cla All CLA requirements met.
Details
roslyn-CI Build #20190918.23 succeeded
Details
roslyn-CI (Linux_Test coreclr) Linux_Test coreclr succeeded
Details
roslyn-CI (SourceBuild_Test) SourceBuild_Test succeeded
Details
roslyn-CI (Windows_CoreClr_Unit_Tests debug) Windows_CoreClr_Unit_Tests debug succeeded
Details
roslyn-CI (Windows_CoreClr_Unit_Tests release) Windows_CoreClr_Unit_Tests release succeeded
Details
roslyn-CI (Windows_Correctness_Test) Windows_Correctness_Test succeeded
Details
roslyn-CI (Windows_Desktop_Spanish_Unit_Tests) Windows_Desktop_Spanish_Unit_Tests succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests debug_32) Windows_Desktop_Unit_Tests debug_32 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests debug_64) Windows_Desktop_Unit_Tests debug_64 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests release_32) Windows_Desktop_Unit_Tests release_32 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests release_64) Windows_Desktop_Unit_Tests release_64 succeeded
Details
roslyn-CI (Windows_Determinism_Test) Windows_Determinism_Test succeeded
Details
roslyn-CI (macOS_Test) macOS_Test succeeded
Details
roslyn-integration-CI Build #20190918.23 succeeded
Details
roslyn-integration-CI (VS_Integration debug_async) VS_Integration debug_async succeeded
Details
roslyn-integration-CI (VS_Integration release_async) VS_Integration release_async succeeded
Details
@ivanbasov ivanbasov deleted the ivanbasov:removelegacy branch Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.