Skip to content

Coding guidelines cibuild #962

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

Merged
merged 18 commits into from
Mar 8, 2021
Merged

Coding guidelines cibuild #962

merged 18 commits into from
Mar 8, 2021

Conversation

bart-degreed
Copy link
Contributor

@bart-degreed bart-degreed commented Mar 3, 2021

This PR contains an automated reformat of the entire codebase. From here on, we intend to follow stricter coding style rules. During cibuild, we run validations to verify compliance. These intentionally break the build on violations (examples here and here).

For local development, we've added inspectcode.ps1 and cleanupcode.ps1 scripts to help with that. See updated contributing guidelines, which is part of this PR.

How it works

We're using the open-source command-line runners from ReSharper for this. Code layout rules and severities are stored in JsonApiDotNetCore.sln.DotSettings (compatible settings are in .editorconfig too, for users not using Resharper or Rider). Where inspectcode scans for violations, cleanupcode just reformats the solution based on layout settings. During cibuild, we run cleanupcode against the changed files from the merge and fail the build if it alters any files.

Fixes #290.
Fixes #835.

I've tried to enable nullable reference types on the solution, but this turned out to be painful for the reasons below, so I reverted that.

  • JADNC targets .NET Core 3.1, whose framework assemblies are mostly unannotated. Most of the annotation was done in .NET 5.
  • The language version used for .NET Core 3.1 is C# 8, which does not support nullability of unconstrained generic type parameters. This is quite painful, because we highly depend on generics. Although I've raised this concern back in 2016, it took until C# 9 to become available. Although it is technically possible to use C# 9 with .NET Core 3.1, it is not recommended: "Choosing a language version newer than the default can cause hard to diagnose compile-time and runtime errors."

Using multi-targeting Net5/NetCore31 (in that order!) would give us the framework annotations from .NET 5. But multi-targeting has its downsides. We'd produce a NuGet with two binaries, we'd run all our tests twice, we'll need to deal with the fact that EF Core 3.x does not run on .NET 5 etc. All-in-all, I don't think this is the right time to make the transition and we should wait until JADNC vNext where we can drop support for NetCore3.1.

I've thought to just annotate the public surface area of our API as recommended here. But due to historical reasons, almost all code is public, so this requires nearly the same amount of effort compared to annotating the entire codebase.

@bart-degreed bart-degreed force-pushed the coding-guidelines-cibuild branch 2 times, most recently from 67dd09b to 151f631 Compare March 4, 2021 16:45
@bart-degreed bart-degreed force-pushed the coding-guidelines-cibuild branch from 151f631 to 441a235 Compare March 4, 2021 16:57
@bart-degreed bart-degreed marked this pull request as ready for review March 4, 2021 17:42
@bart-degreed bart-degreed requested a review from maurei March 4, 2021 17:42
@bart-degreed bart-degreed merged commit 6e4ba03 into master Mar 8, 2021
@bart-degreed bart-degreed deleted the coding-guidelines-cibuild branch March 8, 2021 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add formatting guidelines with PR validation Improve Contributing Guide
2 participants