Add linting and formatting

[cj-young]

Formatting and Linting System Configuration

Resolves #1197

This PR is intended to set up the rules for linting and formatting for both the frontend and the backend. Neither formatting issues nor linting issues have been fixed here. Ideally, developers can merge these rules into their own branches so that when a future PR modifies almost every file, they can just run the formatting command themselves to avoid merge conflicts.

Running the Formatters and Linters

The following Make commands are available to format and lint:

• frontend-fmt: Format all frontend (or frontend-adjacent file-type, like YAML, JSON, etc.) code
• frontend-lint: Lint all frontend code
• backend-fmt: Format all backend code
• backend-lint: Lint all backend code
• format: Format both frontend and backend
• lint: Lint both frontend and backend

Rule and tool choices

Prettier

Prettier is the industry-standard formatter for frontend applications. Because it is opinionated, few rule choices had to be made. Only two options were used:

Rule Name	Value	Justification
experimentalTernaries	true	This option is intended to become the default in Prettier. It makes nested ternaries far more readable.
experimentalOperatorPosition	true	This moves operators in long math expressions to the next line instead of being at the end of the current line. This makes the operator more easily readable up front and therefore makes the entire expression easier to read.

Other options were kept as defaults. Here are some brief justifications for some of those decisions:

Rule Name	Value	Justification
semi	true	Enforces semicolons, which makes statements slightly more readable. Since Prettier is aware of the edge cases where semicolons are necessary, it can insert "defensive semicolons" if semicolons are turned off, so semicolons could be turned off without harm. Because of this, the decision to keep them on is mostly an aesthetic choice.
trailingComma	"all"	Enforces trailing commas, making diffs cleaner. "es5" is another options, but since ES6 features are used anyway, there is no reason to limit trailing commas to be ES5-compatible.

ESLint

ESLint is the industry-standard linter for JS. It has by far the most support and largest ecosystem of all linters. A few plugins are used, namely eslint-plugin-react (rules prefixed with react/), eslin-plugin-react-hooks (rules prefixed with react-hooks/), and eslint-config-prettier (prevents rule conflicts with Prettier). Many of the rules have been given a severity of warn because there are currently so many errors in the codebase, and changes will have to be adopted relatively gradually. Once the lint errors start to be cleared up, most of the warn severities will be updated to error.

A brief justification of each changed rule is as follows:

Rule Name	Value	Justification
no-unused-vars	warn, ignore pattern of ^_	This defaults to an error in ESLint. However, there are currently so many instances of this error that it has been lowered to a warning. It is currently a lower priority since unused variables don't inherently cause broken code, so it should wait to be elevated to an error if elevated at all in the future.
react/prop-types	off	React PropTypes are not used so should not be checked.
react/react-in-jsx-scope	off	React does not need to be imported because webpack does not require it.
react/function-component-definition	error, namedComponents: function-declaration, unnamedComponents: arrow-function	This ensures consistency across the codebase. Function declaration components are already used almost every time, and arrow functions are used almost every (if not every) time with unnamed components. It does not need to be reduced to a warn because there are so few instances of the violation and they are extremely easy to fix.
react/jsx-key	warn, checkFragmentShorthand: true, warnOnDuplicates: true	This ensures every component (and fragment) has a unique key prop. This fixes React warnings and ensures components are correctly tracked when their orders are changed or components are mounted/unmounted.
react/no-array-index-key	warn	Array index keys should never be used as key props. The same explanation for react/jsx-key applies here. This should have a severity of error in the future, but it is so common in the code currently that it is a warn for now.
react/checked-requires-onchange-or-readonly	warn	This ensures that an <input> with type="checkbox" either has a readonly attribute or an onChange function if it is controlled. It is mostly to suppress React warnings.
react-hooks/set-state-in-effect	warn	By default, this is an error. However, there are likely cases where a state setter in a useEffect is necessary, so it has been reduced to a warn. There are many violations in the codebase that should be changed, and there likely will be in the future, so it is not omitted entirely.

PHP CS Fixer

PHP CS Fixer is a PHP formatter created and maintained by the same team that maintains Symfony. Because of this, it integrates easily with Symfony and has lots of integrations and support.

The only rule that is configured separately from the defaults is phpdoc_summary which has been set to false. By default, it adds a period to the end of the first line of PHPDocs, which is somewhat invasive.

PHPStan

PHPStan is the most popular choice for static analysis tools in Symfony applications. It easily integrates with Symfony through an extension and allows easy incremental adoption through its level system and its baseline feature. The standard PHP linter only tracks syntax errors. PHPStan introduces type checking. While types in this codebase are somewhat underused, it is still helpful in tracking possible nullable types, invalid method calls, and type checking where types are implemented. Moving forward, it will encourage the adoption of explicit typing throughout the entire codebase, which will make the app less prone to bugs.

The only major parameter that has been changed is the level, which has been set to 5 (on a 0 to 10 scale, 10 being the most strict). This has been chosen somewhat arbitrarily. There are roughly 150 errors at level 5, and at level 6 that shoots up to around 850 errors, almost all of which are due to missing types. Lowering the level much more wouldn't reduce the amount of errors by a substantial amount when considering the tradeoff of having very loose checks.

No baseline has been created. The baseline should be created once a PR to fix most linting errors is created to reduce the size of the baseline. Gradually, as the codebase becomes more type safe and up to standard, the level can be increased.

Adopting the Formatter

If we take the approach of adopting the correct formatting by incrementally formatting each file only when they are modified and pushed, the git blame on any formatted line will point to whoever ran the formatter and not to whoever originally changed the line, which isn't super useful. Instead, if a single commit (or a handful of commits) dedicated only to formatting are created, a .git-blame-ignore-revs file can be created to ignore these commits in the git blame. While this won't be perfect (since some line additions and line swaps can't be ignored correctly through this method), it will preserve a large majority of the blame.

Eventually, CI checks should be added to ensure formatting and linting are up to the standard of the rest of the codebase. Ideally, this would be implemented right after the PRs that format and lint the entire codebase.

Adopting the Linter

Because lint issues typically change the functionality of the app, the git blame should not ignore lint changes. The commits in the PR that fixes the bulk of the lint issues, therefore, shouldn't be in the .git-blame-ignore-revs. Because lint changes aren't just stylistic in nature, merge conflicts are essentially inevitable. The only ones that could be fixed ahead of time would be through automatically fixable errors. If a process like this is run, it would be helpful to be listed in the PR where they are fixed so other developers know to run the command to reduce the likelihood of merge conflicts.