Proposed new conventions

[flakey-bit]

Hey 👋

I was thinking about contributing back to the project with some conventions that I've used in various repos at my organisation. But before I open a PR (or PRs), I thought I'd check if these conventions would fit the project goals - happy to cherry-pick 🙂

Also, are tests for the conventions a must-have or a nice-to-have?

MvcControllersMustNotUseForbid

What does it do: Checks that controllers deriving from (ASP.NET Core ControllerBase) don't call the Forbid virtual method.

Why would you want that: If you're relaying forbidden from an upstream API you might want to avoid interactions with ASP.NET authentication code. You also might want to return a ProblemDetails response

• Derives from MustNotCallMethodConventionSpecification
• Would introduce a dependency on Microsoft.AspNetCore.Mvc.Core

Probably not generally applicable to be honest, but I thought I'd check

MustNotUseGuidNewGuid

What does it do: Checks that code doesn't call Guid.NewGuid()

Why would you want that: If you're trying to avoid random data and/or you're trying to write things in a functional way.

• Derives from MustNotCallMethodConventionSpecification
• No dependencies

NamespaceMustStartWith

What does it do: Checks that the type is in a namespace beginning with some needle

Why would you want that: In case MustLiveInNamespaceConventionSpecification is not flexible enough for you.

We're using it to partition a namespace e.g.

private const string RequestDtoNamespacePrefix = "Acme.Foobar.BFF.DTOs.Request";
private const string ResponseDtoNamespacePrefix = "Acme.Foobar.BFF.DTOs.Response";

typeof(StatementLine).Assembly.GetExportedTypes()
    .MustConformTo(new NamespaceMustStartWith(RequestDtoNamespacePrefix)
        .Or(new NamespaceMustStartWith(ResponseDtoNamespacePrefix)))
    .WithFailureAssertion(ConventionHelpers.Fail);

• Derives from ConventionSpecification
• No dependencies

AssembliesMustBeIncludedInListConventionSpecification

Derives from AssemblyConventionSpecification
What does it do: Checks that an assembly (name) is included in a hard-coded list/set (of assemblies)

Why would you want that: To help synchronise type-level convention checking

For example

// SolutionTests.cs

private AssemblySpecimen[] NonTestApiAssemblies =>
    AllSolutionAssemblies
        .Where(IsNonTestProject)
        .ToArray();

[Theory]
[MemberData(nameof(EnumerateWarningsToBeTreatedAsErrors))]
public void NonTestProjectsMustTreatWarningAsError(string warning)
{
    NonTestApiAssemblies
        .ToArray()
        .MustConformTo(new TreatsWarningAsErrorConventionSpecification(warning))
        .WithFailureAssertion(ConventionHelpers.Fail);
}

[Fact]
public void NonTestProjectsMustBeIncludedInNonTestAssembliesList()
{
    // We have an (admittedly small) set of convention tests we want to run across all non-test assemblies
    // This test enforces that list of assemblies is up-to-date
    NonTestApiAssemblies
        .ToArray()
        .MustConformTo(
            new AssembliesMustBeIncludedInListConventionSpecification(
                CommonConventionTests.NonTestAssemblies,
                $"{nameof(CommonConventionTests)}:{nameof(CommonConventionTests.NonTestAssemblies)}"
            ))
        .WithFailureAssertion(ConventionHelpers.Fail);
}

// CommonConventionTests.cs

[Theory]
[MemberData(nameof(EnumerateNonTestAssemblies))]
public void Convention_MustNotAccessNewRelicAgentStatically(Assembly assembly)
{
    // NewRelic agent (IAgent) should be injected rather than accessed statically via NewRelic.GetAgent()
    assembly.GetTypes()
        .Where(t => ((t.DeclaringType ?? t) != typeof(Startup)))
        .Where(ConventionHelpers.TypeIsNotGeneratedByCoverlet)
        .MustConformTo(new MustNotAccessNewRelicAgentStatically())
        .WithFailureAssertion(ConventionHelpers.Fail);
}

public static IEnumerable<object[]> EnumerateNonTestAssemblies => NonTestAssemblies.Select(a => new[] { a });

AssemblyReferenceConventionSpecification

Abstract type

MustReferenceAssemblyConventionSpecification

What does it do: Checks that an assembly includes a reference to another assembly (either project reference or package reference)

Derives from AssemblyReferenceConventionSpecification

MustNotReferenceAssemblyConventionSpecification

What does it do: Checks that an assembly does not include a reference to another assembly

Derives from AssemblyReferenceConventionSpecification

MustNotIncludeProjectReferencesConventionSpecification

What does it do: Checks that a project doesn't reference any other projects
Why would you want that: Avoiding bringing in types unintentionally

Derives from AssemblyReferenceConventionSpecification

ProjectMustSpecifyPropertyValueConventionSpecification

What does it do: Checks that a project sets a property in the .csproj

TreatsWarningAsErrorConventionSpecification

What does it do: Checks that a project treats a given warning as an error (via WarningsAsErrors)
Why would you want that: Perhaps you want to enforce that non-test projects treat specific warnings (e.g. possible null reference) as errors.

[andrewabest]

👋 hello @flakey-bit!

I was thinking about contributing back to the project

🙇

Also, are tests for the conventions a must-have or a nice-to-have?

Must-have, but if you are struggling to test something, let's talk about why. The tests also help people see how to hold the convention.

Let's go through the potential contributions in the order you've given them:

MvcControllersMustNotUseForbid ❌

I'd prefer to avoid taking the aspnet dependency. I've already done this with System.Data.SqlClient, and it has caused issues. If I saw a bigger need for these types of conventions I'd probably create a Best.Conventional.Aspnet and Best.Conventional.Sql. Right now I'm living with the sql dependency, but I'd prefer to avoid the aspnet one.

MustNotUseGuidNewGuid ✅

This sounds fine, as you've pointed out the infrastructure required to write it is right there to use 👍

NamespaceMustStartWith ✅

AssembliesMustBeIncludedInListConventionSpecification ✅

MustReferenceAssemblyConventionSpecification ✅

MustNotReferenceAssemblyConventionSpecification ✅

MustNotIncludeProjectReferencesConventionSpecification ✅

ProjectMustSpecifyPropertyValueConventionSpecification ✅

TreatsWarningAsErrorConventionSpecification ✅

These all sound like useful additions for controlling assembly-level concerns 👍

Thanks for reaching out before throwing up a PR!

[flakey-bit]

@andrewabest I've opened #89