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

Disable cake diagnostics endpoint for requests that don't specify a file #1107

Merged
merged 6 commits into from
Feb 12, 2018

Conversation

rchande
Copy link

@rchande rchande commented Feb 7, 2018

Cake's diagnostics engine calls the generic CodeCheckService, which generates all diagnostics for the solution if called without a specific file path. This results in duplicate errors in C# projects when VS Code requests project level diagnostics. The solution is to only compute diagnostics for specified files.

Fixes dotnet/vscode-csharp#1830

Diagnostics window in VS Code now looks like:
image

cc @mholo65

Cake's diagnostics engine calls the generic CodeCheckService, which generates all diagnostics for the solution if called without a specific file path. This results in duplicate errors in C# projects when VS Code requests project level diagnostics. The solution is to only compute diagnostics for specified files.

Fixes dotnet/vscode-csharp#1830
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.

Don't think it's necessary to introduce a new virtualmethod in CakeRequestHandler since Handle is already virtual.


return await TranslateResponse(response, request);
}

public virtual Task<TResponse> HandleCore(TRequest request, IRequestHandler<TRequest, TResponse> service)
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to introduce this? Task<TResponse> Handle(TRequest request) is already virtual.

@@ -14,5 +15,15 @@ public class CodeCheckHandler : CakeRequestHandler<CodeCheckRequest, QuickFixRes
: base(workspace)
{
}

public override Task<QuickFixResponse> HandleCore(CodeCheckRequest request, IRequestHandler<CodeCheckRequest, QuickFixResponse> service)
Copy link
Member

Choose a reason for hiding this comment

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

Override Task<TResponse> Handle(TRequest request) instead.

return Task.FromResult(new QuickFixResponse());
}

return service.Handle(request);
Copy link
Member

Choose a reason for hiding this comment

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

Override Task Handle(TRequest request) and call base.Handle(request) instead?

@rchande
Copy link
Author

rchande commented Feb 7, 2018

@mholo65 It seems like Handle() does a bunch of pre- and post-processing on the request and result type that every overrider shouldn't have to be concerned with. I therefore added HandleCore where endpoints only have to be concerned with the actual call to the underlying service. Thoughts?

@bjorkstromm
Copy link
Member

@rchande that’s a valid point. Instead of adding a new HandleCore method, what do you think of adding a bool IsValid(TRequest request) with the intention to validate the request?

@rchande
Copy link
Author

rchande commented Feb 7, 2018

@mholo65 Good idea, I'll do that.

var diagnostics = await FindDiagnostics(input, includeFileName: false);
Assert.Null(diagnostics);
}
private async Task<QuickFixResponse> FindDiagnostics(string contents, bool includeFileName)
Copy link
Contributor

Choose a reason for hiding this comment

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

blank line above

@@ -54,11 +54,15 @@ protected CakeRequestHandler(OmniSharpWorkspace workspace)

request = await TranslateRequestAsync(request);

var response = await service.Handle(request);
var response = IsValid(request)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe check validity before TranslateRequest?

@rchande rchande merged commit 11e72a5 into OmniSharp:master Feb 12, 2018
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