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

Codeactions with options/ui. #1406

Merged
merged 58 commits into from
Jul 10, 2019

Conversation

savpek
Copy link
Contributor

@savpek savpek commented Feb 21, 2019

Resolves #1220

List:

  • ChangeSignatureCodeAction
    • Todo: blacklist this, i don't think this is possible to impelement without UI...
  • ExtractInterfaceCodeAction
  • GenerateConstructorWithDialogCodeAction
  • GenerateOverridesWithDialogCodeAction
  • GenerateEqualsAndGetHashCodeWithDialogCodeAction
  • GenerateTypeCodeActionWithOption
    • Blacklist. This don't give extra value over GenerateType (without options) which we have already. With default ui this ends up nearly identical class.
  • PullMemberUpWithDialogCodeAction
    • This is bit trickier, may work OK if select first available parent interface as target.
    • Blacklist: Theres existing pull members up without dialog. Use that one and blacklist this, this is identical when used without UI.

There are some parts that are internal that are reguired to implement this feature. Pending issue on dotnet/roslyn#33277. This is written with reflection/proxy combination which should be replaced once roslyn issue is addressed.

GenerateConstructorWithDialogCodeAction, GenerateOverridesWithDialogCodeAction, GenerateEqualsAndGetHashCodeWithDialogCodeAction:
code_actions_with_options

Extract interface:
code_actions_with_options_extract_interface

However extract interface isn't available before this code is merged with #1076 since it's action for one of analysis.

@savpek savpek changed the title Feature/codeactions with options [WIP] Feature/codeactions with options Feb 21, 2019
@savpek savpek changed the title [WIP] Feature/codeactions with options Codeactions with options/ui. Feb 24, 2019
@savpek savpek mentioned this pull request Feb 28, 2019
5 tasks
@david-driscoll
Copy link
Member

I'm going to focus on getting this and the related analyzers in right away.

Copy link
Member

@bjorkstromm bjorkstromm left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. We probably want to avoid a dependency on Castle.Core. Take a look at MetadataHelper on how it's done there.

@@ -16,6 +16,7 @@
<PackageReference Include="Microsoft.Extensions.Caching.Memory" />
<PackageReference Include="Microsoft.CodeAnalysis.Common" />
<PackageReference Include="System.ComponentModel.Composition" />
<PackageReference Include="Castle.Core" Version="4.3.1" />
Copy link
Member

Choose a reason for hiding this comment

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

We probably want to avoid a dependency for Castle.Core?

{
public class ExtractInterfaceWorkspaceService : IInterceptor
{
public void Intercept(IInvocation invocation)
Copy link
Member

Choose a reason for hiding this comment

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

To avoid a dependency on Castle.Core, you can have a look in MetadataHelper on how this could be done.

@savpek
Copy link
Contributor Author

savpek commented Mar 19, 2019

In metadata helper concrete internal type is created (if i am not missing something completly 😄) but this is bit trickier since concrete implementation of internal interface is created without existing concrete implementation. Implementation for IPickMembersService for is in visual studio libraries and not available.

However it think i can write solution based on https://stackoverflow.com/questions/8606412/create-type-that-implements-internal-interface which looks something like example there:

public class InterfaceImplementer : RealProxy, IRemotingTypeInfo
{
    readonly Type _type;
    readonly Func<MethodInfo, object> _callback;

    public InterfaceImplementer(Type type, Func<MethodInfo, object> callback) : base(type)
    {
        _callback = callback;
        _type = type;
    }

    public override IMessage Invoke(IMessage msg)
    {
        var call = msg as IMethodCallMessage;

        if (call == null)
            throw new NotSupportedException();

        var method = (MethodInfo)call.MethodBase;

        return new ReturnMessage(_callback(method), null, 0, call.LogicalCallContext, call);
    }

    public bool CanCastTo(Type fromType, object o) => fromType == _type;

    public string TypeName { get; set; }
}

Which doesn't look too bad to write and should remove requirement for castle.core depency.

@bjorkstromm
Copy link
Member

@david-driscoll @filipw and @rchande, any opions on the above suggestion by @savpek? One problem is that neither RealProxy nor IRemotingTypeInfo is part of netstandard (according to https://apisof.net/)

@savpek
Copy link
Contributor Author

savpek commented Mar 20, 2019

I will try with DispatchProxy which seems to be part of net standard.

@david-driscoll
Copy link
Member

One minor nit, otherwise we're good to go.

@savpek
Copy link
Contributor Author

savpek commented Jun 26, 2019

@david-driscoll can you point me to that minor nit, i don't find any comment or anything for it 🙂

@david-driscoll
Copy link
Member

Looks like you fixed it 🎉

@david-driscoll david-driscoll dismissed rchande’s stale review June 27, 2019 17:19

Review concerns were met

Copy link
Member

@filipw filipw left a comment

Choose a reason for hiding this comment

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

LGTM

@savpek
Copy link
Contributor Author

savpek commented Jul 10, 2019

@filipw @david-driscoll is anything holding this PR back at this point? Its bit burder to maintain PRs for extended period of time 🙂

@filipw
Copy link
Member

filipw commented Jul 10, 2019

sorry for the delay - this is good to go - thanks a lot for your hard work 👍

@filipw filipw merged commit 8978783 into OmniSharp:master Jul 10, 2019
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.

Support for code actions/refactorings that show UI in Visual Studio
5 participants