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

Clean up code action service and rationalize how we disallow certain Roslyn code actions #752

Merged

Conversation

DustinCampbell
Copy link
Contributor

Fixes dotnet/vscode-csharp#925

There's a fair amount of refactoring in this PR but most of it is moving code around.

Copy link
Member

@david-driscoll david-driscoll left a comment

Choose a reason for hiding this comment

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

Looks good, if you want to remove the NRefactory list you can though.

{
public static class LoggerExceptions
{
public static void LogError(this ILogger logger, Exception ex, string message, params object[] args)
Copy link
Member

Choose a reason for hiding this comment

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

I hate that this overload doesn't exist.

Copy link
Member

Choose a reason for hiding this comment

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

i think the intent was to use const "categories" as event IDs, but yeah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I hate the event ID thing. Should be optional. There's no reason to have a "0" everywhere if you aren't using them. 😄

};

private static readonly HashSet<string> _nrefactoryListToRemove = new HashSet<string>
{
"ICSharpCode.NRefactory6.CSharp.Refactoring.CreateMethodDeclarationAction",
Copy link
Member

Choose a reason for hiding this comment

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

Can probably just drop this list for now (perhaps even the hashset).

Currently Refactoring Essentials isn't built with the latest roslyn dlls, so the plugins branch fails. I'll have to get some other form of unit test in there to ensure that plugins are working.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@DustinCampbell DustinCampbell merged commit 004ed15 into OmniSharp:dev Feb 5, 2017
@DustinCampbell DustinCampbell deleted the rationalize-code-action-services branch February 8, 2017 01:26
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.

3 participants