-
Notifications
You must be signed in to change notification settings - Fork 648
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
base: main
Are you sure you want to change the base?
feat: Analyzers #1240
Conversation
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. |
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. |
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. |
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 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}"); |
Made some more progress discussing with the maintainer of GuAnalyzers some best practices. |
Awesome, sorry I've been AWOL, took a couple of weeks off. Happy to chip in when/where I can :) |
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. |
fixes: #1184