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

feat: Analyzers #1240

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

benmccallum
Copy link

fixes: #1184

@jzabroski
Copy link
Collaborator

I need to add a nuget.config file to the solution to get this to pull in Microsoft.CodeAnalysis.CSharp.Analyzer.Testing as a dependency. @schambers You mentioned wanting some low hanging fruit - @benmccallum wrote this PR awhile back and it's pretty cool what it does. Can you add a nuget.config with the Roslyn package source?

See: https://github.com/dotnet/roslyn-sdk/tree/master/src/Microsoft.CodeAnalysis.Testing#azure-packages-feed-for-prerelease-packages for the updated nuget.config instructions - in September, Microsoft killed off MyGet and moved to Azure Dev Ops package hosting.

@benmccallum
Copy link
Author

This was unfortunately left pretty raw as I was stuck trying to get some form of test runs happening to TDD it but I'm sure with a bit of massaging and newer versions of things it'd be fixable and then you could start actually writing an analyser :p

I think the general core analyser helpers that I wrote are valuable; I was using xunit's analyser stuff as heavy inspiration.

@jzabroski
Copy link
Collaborator

I hope to give it a shot soon. I think the key thing is to add a nuget.config so that whoever wants to build on top of what you did, can. I have had some more ideas for analyzers since you wrote this PR, so it was a good start to get us thinking in the direction of cleaner code and more static analysis.

@jzabroski
Copy link
Collaborator

I made some progress editing your PR.

I have not pushed any changes yet. There are some things about Roslyn that I don't quite understand, like this todo [jaz] note I created:

        internal override void AnalyzeCompilation(CompilationStartAnalysisContext compilationStartContext, FluentMigratorContext fluentMigratorContext)
        {
            compilationStartContext.RegisterSymbolAction(symbolContext =>
            {
                var typeSymbol = (ITypeSymbol)symbolContext.Symbol;
                var migrationVersion = GetMigrationAttributeVersion(fluentMigratorContext, typeSymbol.GetAttributes());
                if (migrationVersion == null)
                {
                    return;
                }

                var otherTypeNames =
                    symbolContext.Compilation.Assembly.TypeNames.Where(typeName => typeName != typeSymbol.MetadataName).ToList();
                foreach (var otherTypeName in otherTypeNames)
                {
                    var ns = symbolContext.Symbol.ContainingNamespace.Name;
                    // todo [jaz]: Fix me. I don't think this is the right way to get the namespace, but without this, the tests don't pass at all right now,
                    // because GetTypeByMetadataName requires a fully qualified type name.
                    var otherTypeSymbol = symbolContext.Compilation.GetTypeByMetadataName($"{ns}.{otherTypeName}");

@jzabroski
Copy link
Collaborator

Made some more progress discussing with the maintainer of GuAnalyzers some best practices.
Still not clear about a few things, but have enough info to hopefully push forward with something I can commit soon.

@benmccallum
Copy link
Author

Awesome, sorry I've been AWOL, took a couple of weeks off. Happy to chip in when/where I can :)

@jzabroski
Copy link
Collaborator

I haven't been able to make progress over the holidays due to trying to wrap up things. My current contract ends in February so its top priority to finish strong. If you're interested I can push what I have to a branch on jzabroski/FluentMigrator.

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.

Analyzer that warns of duplicate MigrationAttribute version values
2 participants