Introduction

All Ed-Fi source code should have a linter built in for detecting common style bugs and enforcing style consistency. While these tools can sometimes be seen as an annoyance, they frequently identify real problems in code; they train developers in best practices; and they create a level playing field for all contributors, regardless of experience with the language or with the project norms.

Basic requirements for a project linter:

1. MUST: be configurable
2. MUST: be runnable at the command line
3. MUST: be runnable in GitHub Actions
4. MUST: be free to use
5. SHOULD: be integrated into common IDEs, or have equivalent replacements that use the same rules.

C#

Historically, the Alliance has relied on ReSharper for detecting common code smells, style inconsistencies, and more. However, it does not meet the requirements above for a good linter in several respects (see Background Research - C# Linter). Instead, the Alliance is adopting a combination of two tools:

• Microsoft's .NET compiler platform analyzers (Roslyn), and 
• SonarLint by SonarSource (makers of SonarQube), which builds on the Roslyn framework.

In combination with a .editorconfig file, these tools provide robust analysis and reporting, whether from the command line, Visual Studio, or Visual Studio Code.  Furthermore, the .editorconfig rules provide the configuration needed to run the dotnet format command for automatic reformatting of source code files.

Customization

The style rules can be tweaked by the Ed-Fi Tech Team with careful review. The following articles are helpful in understanding the various options:

• Analyze Configuration (Overview)
  
  • Language Rules
  • Naming Rules
  • Quality Rules
• SonarLint C# Rules

Usage

We install these analyzers as NuGet package dependencies in C# projects. For each project*:

cd <project directory>
dotnet add package Microsoft.CodeAnalysis
dotnet add package Microsoft.CodeAnalysis.CSharp.CodeStyle
dotnet add package SonarAnalyzer.CSharp

* note: to reduce scope of changes, it is recommended to apply this to one or a small number of projects at time; see migration plan below.

Next, copy the appopriate .editorconfig file and the Directory.Build.props file from the DevSecOps/dotnet tools. Paste them into the same directory as the solution file. If there is already a build props file, update it to include the extra two lines in this sample (treating warnings as errors, and outputting a sarif log file).

Finally, for open source projects, adjust the GitHub Actions build configuration by adding step(s) to upload sarif log file into the CodeQL results. This will integrate any build failure messages into GitHub for easy viewing. This feature requires a substantially more expensive license for the closed source repositories and thus will not be used.

Example Output

When you run msbuild at the command line, warnings will now  be treated as errors - thus causing the build to fail, and giving us strict enforcement.

...
Build FAILED.

C:\source\ed-fi\ed-fi-ods-implementation\Application\EdFi.Ods.SandboxAdmin\Controllers\Api\SandboxControll 
er.cs(13,18): error S101: Rename class 'SandboxDTO' to match pascal case naming rules, consider using 'San 
dboxDto'. [C:\source\ed-fi\ed-fi-ods-implementation\Application\EdFi.Ods.SandboxAdmin\EdFi.Ods.SandboxAdmi 
n.csproj]
    0 Warning(s)
    1 Error(s)

Time Elapsed 00:00:46.71

Disabling Warnings

Most of the recommendations should be followed closely. However, there will be cases where the code cannot be changed, it is impractical to test the change, or we simply have a well-reasoned disagreement. Within Visual Studio it is easy to right-click on the problem and "disable it". Please put in a custom comment explaining why the issue is not being dealt with. The code reviewer should inspect that line of reasoning; if there is disagreement, then ask a third person to join the conversation. In the following example, the comment starts with the code smell "Types should be named in PascalCase", and continues with our own comment explaining why we disabled the warning.

#pragma warning disable S101 // Types should be named in PascalCase, however, uppercase is preferred for acronyms such as "DTO"
    public class SandboxDTO
#pragma warning restore S101 // Types should be named in PascalCase

Migration Plan

If a repository has more than 2-3 projects, then introducing these rules and updating the code to correct the warnings will take some time to code, test, and review. Thus it is recommended to introduce the analyzers to only 2-3 projects - or, only a single project if it has many files (e.g. EdFi.Ods.Api).

After adding the analyzers, .editorconfig , and Directory.Build.props , it may be useful to run the dotnet format  command on the projects (not the entire solution) as a first step, and commit just those file changes. Please note that many files will experience what will look like strange churn, due to changes at the top and bottom of the file:

• this will strip out a UTF byte order mark (BOM), if present
• and it will ensure there is an extra line break at the end of the file, which helps view the file correctly in Unix-like environments.

Proof-of-Concept Spikes

The following pull requests show proof-of-concept spikes in several different repositories. Note that the Data Import spike is in a closed source repo and only a small number of developers can access it.

• Analytics-Middle-Tier - this includes integration of Sarif log files into CodeQL.
• Ed-Fi-ODS-Implementation
• Data-Import

JavaScript and TypeScript

JavaScript has long had excellent tools for style and rule enforcement. Following the lead of the MetaEd project, JavaScript and TypeScript projects must use

1. the Airbnb JavaScript Style Guide, and enforce that style with ESLint using Airbnb's rules in eslint-config-airbnb.
2. Prettier for code formatting and enforce that via ESLint using eslint-plugin-prettier  and eslint-config-prettier .

Usage

Install the following packages as dev dependencies:

• @types/eslint
• @typescript-eslint/eslint-plugin
• @typescript-eslint/parser
• eslint
• eslint-config-airbnb-base
• eslint-config-prettier
• eslint-config-typescript
• eslint-plugin-import
• eslint-plugin-jasmine
• eslint-plugin-json
• eslint-plugin-prettier
• prettier

And then add copy the appropriate configuration files from DevSecOps/JavaScript tools.

Python

Like JavaScript, the Python community has a long history of good tooling. Following the lead of the LMS Toolkit project, other Python code must use:

1. Flake8 for style enforcement
2. MyPy for type checking
   1. Pylance in VS Code will follow the same configuration
3. Black for formatting

Usage

Install the following packages as dev dependencies:

• flake8
• mypy
• black

And then add copy the appropriate configuration files from DevSecOps/Python tools.

PowerShell

For PowerShell, we can use the PSScriptAnalyzer to review the scripts and modules.

To do so, clone: https://github.com/Ed-Fi-Alliance-OSS/Ed-Fi-Actions/tree/main/powershell-analyzer and run analyze.ps1

This script takes different options as parameters, the required parameter is the -Directory where you can specify a file or a folder, and this will review the file or folder and subfolders included.

The flag -SaveToFile defines if it should only print the results into the console or save to a SARIF file. The SARIF file can be viewed in VSCode with an extension.

Additionally, you can specify the path where to save the results in the -ResultsPath parameter or specify a comma separated list of rules to be excluded with the -ExcludedRules parameter.

Example:

.\analyze.ps1 -Directory "Ed-Fi-ODS-AdminApp\eng\send-test-results.ps1" -SaveToFile $False -ExcludedRules PSReviewUnusedParameter, PSAvoidUsingWriteHost

If a warning should be ignored, annotate your class or function with the following:

[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments', 'unused', Justification = 'False positives')]

List of rules: https://learn.microsoft.com/en-us/powershell/utility-modules/psscriptanalyzer/rules-recommendations?view=ps-modules