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

Fix diagnostics from loaded cake files shown in the current file #1201

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
# Changelog
All changes to the project will be documented in this file.

## [1.31.1] - _Not Yet Released_
* Fixed bug where diagnostics from loaded `.cake` files was shown in the current file. (PR: [#1201](https://github.com/OmniSharp/omnisharp-roslyn/pull/1201))

## [1.31.0] - 2018-05-29
* Update to Roslyn 2.8.0 packages, adding support for C# 7.3. (PR: [#1182](https://github.com/OmniSharp/omnisharp-roslyn/pull/1182))
* MSBuild project system no longer stops when a project fails to load. (PR: [#1181](https://github.com/OmniSharp/omnisharp-roslyn/pull/1181))
Expand Down
2 changes: 1 addition & 1 deletion src/OmniSharp.Cake/Services/MonoScriptGenerationProcess.cs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public void Start(int port, string workingDirectory)
process.WaitForExit();

// If OmniSharp bundled Mono runtime, use bootstrap script.
var script = Path.Combine(Path.GetDirectoryName(runtime), "run");
var script = Path.Combine(Path.GetDirectoryName(runtime), "../run");
if (File.Exists(script))
{
return (script, "--no-omnisharp ");
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
using System.Composition;
using System;
using System.Composition;
using System.IO;
using System.Linq;
using System.Threading.Tasks;
using OmniSharp.Mef;
using OmniSharp.Models;
using OmniSharp.Models.CodeCheck;
using OmniSharp.Utilities;

namespace OmniSharp.Cake.Services.RequestHandlers.Diagnostics
{
Expand All @@ -18,5 +22,33 @@ public CodeCheckHandler(

public override bool IsValid(CodeCheckRequest request) =>
!string.IsNullOrEmpty(request.FileName);

protected override Task<QuickFixResponse> TranslateResponse(QuickFixResponse response, CodeCheckRequest request)
{
if (response?.QuickFixes == null)
{
return Task.FromResult(response);
}

var quickFixes = response.QuickFixes.Where(x => PathsAreEqual(x.FileName, request.FileName));
response.QuickFixes = quickFixes;
return Task.FromResult(response);

bool PathsAreEqual(string x, string y)
{
if (x == null && y == null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be ReferenceEquals(x, y) to be even more efficient in case the same string instance happens to be reused.

{
return true;
}
if (x == null || y == null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Another tip I had from a contributor at corefx was that x == null | y == null performs better than the branch introduced by ||. Not that this is performance-critical though.

{
return false;
}

var comparer = PlatformHelper.IsWindows ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal;

return Path.GetFullPath(x).Equals(Path.GetFullPath(y), comparer);
}
}
}
}
3 changes: 3 additions & 0 deletions test-assets/test-projects/CakeProject/error.cake
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
class Foo {
public asdf MyProperty { get; set; }
}
4 changes: 2 additions & 2 deletions test-assets/test-projects/CakeProject/tools/packages.config
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<packages>
<package id="Cake" version="0.23.0" />
<package id="Cake.Bakery" version="0.2.0" />
<package id="Cake" version="0.27.2" />
<package id="Cake.Bakery" version="0.3.0" />
</packages>
9 changes: 5 additions & 4 deletions tests/OmniSharp.Cake.Tests/CakeProjectSystemFacts.cs
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,10 @@ public async Task ShouldGetProjects()
{
var workspaceInfo = await GetWorkspaceInfoAsync(host);

Assert.Equal(2, workspaceInfo.Projects.Count());
Assert.Equal(3, workspaceInfo.Projects.Count());
Assert.Contains("build.cake", workspaceInfo.Projects.Select(p => Path.GetFileName(p.Path)));
Assert.Contains("foo.cake", workspaceInfo.Projects.Select(p => Path.GetFileName(p.Path)));
Assert.Contains("error.cake", workspaceInfo.Projects.Select(p => Path.GetFileName(p.Path)));
}
}

Expand All @@ -43,16 +44,16 @@ public async Task ShouldAddAndRemoveProjects()
var tempFile = Path.Combine(testProject.Directory, "temp.cake");

var workspaceInfo = await GetWorkspaceInfoAsync(host);
Assert.Equal(2, workspaceInfo.Projects.Count());
Assert.Equal(3, workspaceInfo.Projects.Count());

await AddFile(host, tempFile);
workspaceInfo = await GetWorkspaceInfoAsync(host);
Assert.Equal(3, workspaceInfo.Projects.Count());
Assert.Equal(4, workspaceInfo.Projects.Count());
Assert.Contains("temp.cake", workspaceInfo.Projects.Select(p => Path.GetFileName(p.Path)));

await RemoveFile(host, tempFile);
workspaceInfo = await GetWorkspaceInfoAsync(host);
Assert.Equal(2, workspaceInfo.Projects.Count());
Assert.Equal(3, workspaceInfo.Projects.Count());
Assert.DoesNotContain("temp.cake", workspaceInfo.Projects.Select(p => Path.GetFileName(p.Path)));
}
}
Expand Down
13 changes: 12 additions & 1 deletion tests/OmniSharp.Cake.Tests/CodeCheckFacts.cs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,18 @@ public async Task ShouldNotCallCodeCheckServiceIfRequestDoesNotSpecifyFileName()
var diagnostics = await FindDiagnostics(input, includeFileName: false);
Assert.Null(diagnostics);
}


[Fact]
public async Task ShouldNotIncludeDiagnosticsFromLoadedFilesIfFileNameIsSpecified()
{
const string input = @"
#load error.cake
var target = Argument(""target"", ""Default"");";

var diagnostics = await FindDiagnostics(input, includeFileName: true);
Assert.Empty(diagnostics.QuickFixes);
}

private async Task<QuickFixResponse> FindDiagnostics(string contents, bool includeFileName)
{
using (var testProject = await TestAssets.Instance.GetTestProjectAsync("CakeProject", shadowCopy : false))
Expand Down