Azure / azure-functions-durable-extension

Durable Task Framework extension for Azure Functions
MIT License
711 stars 263 forks source link

Scaffolding for .NET Isolated Analyzer #2777

Closed allantargino closed 3 months ago

allantargino commented 3 months ago

This PR scaffolds two projects:

Pull request checklist

allantargino commented 3 months ago

Hi @davidmrdavid and @bachuv, thanks for the review! I just fixed the indentation, added copyrights and removed the ` from the file names. They were actually generated by VS template, so I didn't change their names or content. But I do agree the names shouldn't have that special char!

I believe the `1 meant the verifier class accepted one generic argument (just the analyzer, as explained below), and the `2 meant it accepted two generic arguments (the analyzer and its code fixer)

allantargino commented 3 months ago

@bachuv, I am sorry if the files seem a bit disconnected/random, I should have given more context about them, my bad :)

They are all utilities that extend Microsoft.CodeAnalysis.Testing and Microsoft.CodeAnalysis.CSharp.Testing NuGet packages, providing an easier interface for tests. They are created as part of a new Analyzer project template. The Roslyn team provides them to help writing cleaner and simpler unit tests for Analyzers.

For instance, this is how a unit test is written using those built-in utilities (which is pretty close to what I am going to send in my next PR):

using SomeNamespace.Analyzer;
using VerifyCS = Worker.Extensions.DurableTask.Analyzers.Tests.CSharpAnalyzerVerifier<SomeNamespace.Analyzer.SomeAnalyzer>;

namespace SomeNamespace.Tests;

public class UnitTest1
{
    [Fact]
    public async Task NoDiagnostic()
    {
        var test = @"";

        await VerifyCS.VerifyAnalyzerAsync(test);
    }

    [Fact]
    public async Task DateTimeNowHasDiagnostic()
    {
        var test = @"
using System;

class Someclass
{   
    static void Write()
    {
        var now1 = {|#0:DateTime.Now|};
        Console.WriteLine(now1);
    }
}";

        var expected = VerifyCS
            .Diagnostic(SomeAnalyzer.DiagnosticId)
            .WithLocation(0)
            .WithArguments("System.DateTime.Now");

        await VerifyCS.VerifyAnalyzerAsync(test, expected);
    }
}

So, in general terms, the files do:

Giving this context, specially that those files are generated during scaffolding, should we change their names? IMHO, I believe we should keep their names (and content) as close as possible to the generated files, so that when the Roslyn team updates them, we can just replace our ones (and easily relate to the updates).

That's why I think they were created as partial classes, so we can extend them without actually changing the original ones. (to be honest, I don't quite get why those files aren't shipped within the actual NuGet package!)

Thoughts? (I tried to respond all your comments here at once :D but please let me know if I can clarify anything else, I really appreciate the review!)

bachuv commented 3 months ago

@allantargino Thanks for the in-depth explanation! Can you add this information in a README file for the project?

In terms of the file names, I agree that we can leave the names if they are auto generated. Can you take a look at how those files are structured in the existing analyzer project for In Proc though?

allantargino commented 3 months ago

@bachuv, I just simplified the files' structure, to:

Besides, I also added a simple README file with some of the current discussion info (to be further expanded later).

allantargino commented 3 months ago

Thanks for the approval, @davidmrdavid! As per yesterday's internal discussion, we are going to develop the analyzer in the microsoft/durabletask-dotnet repository. The analyzer should be able to analyze not only Durable Functions code, but also usage in DTFx (although initially only focusing on matching parity with the current DF in-proc analyzer).

I am going to close this PR and move it to https://github.com/microsoft/durabletask-dotnet/pull/283 Thanks!