From 5dd03e4d7ebac020bce88a49df5f23efb11b10f6 Mon Sep 17 00:00:00 2001 From: John Whitlock Date: Wed, 3 Apr 2024 16:39:26 -0500 Subject: [PATCH 1/6] ADR 0004 first draft --- docs/adr/0004-use-code-linters.md | 369 ++++++++++++++++++++++++++++++ 1 file changed, 369 insertions(+) create mode 100644 docs/adr/0004-use-code-linters.md diff --git a/docs/adr/0004-use-code-linters.md b/docs/adr/0004-use-code-linters.md new file mode 100644 index 0000000000..62e5fd327a --- /dev/null +++ b/docs/adr/0004-use-code-linters.md @@ -0,0 +1,369 @@ +# Use Code Linters to Enforce Layout and Discover Issues + +- Status: Informational +- Deciders: Luke Crouch +- Date: 2024-04-04 + +Technical Story: [MPP-79][] + +[MPP-79]: https://mozilla-hub.atlassian.net/browse/MPP-79 + +## Context and Problem Statement + +A linter is a static analysis tool that find bugs, style issues, and suspicious +constructs. Some of the areas addressed by different linting tools include: + +- **Enforce Layout** - Checks (and often reformats) code to fit a project + style. This helps make code and code changes easier to read and comprehend. + It also allows new contributors to match the existing project style. +- **Check Types** - Verifies that code is using types consistently. + This is especially useful for dynamic languages where incorrect type errors + would lead to bugs or runtime exceptions. +- **Identify Mistakes** - Detects valid code that is often associated with a + programming mistake. Some examples are identifying unused variables, and + using loose equality in JavaScript. +- **Standardize Constructs** - Picks a way to express a concept, when the + language allows several was to express it. Some examples are import order, + or if default values should be specified by the caller. +- **Spelling and Grammar Checking** - Checks comments in a similar way to a + word processor. Comments often use project-specific technical terms, making + this a different task from checking English prose. + +Most developers appreciate that linters make multi-developer projects easier. +The shared project standards can be checked and enforced by tools. Code +reviewers can focus on the logic of changes, rather than the form of the code. +A linter may identify an unexpected issue. A developer can read the linter +documentation to learn more about the issue. This is one way a developer can +improve their understanding of the project's languages. + +On the other hand, developers resent changes to their working process, +especially when the benefits are unclear or they disagree with the trade-offs. +It becomes harder to add a tool as the lines of code and the number of team +members grow. The best time to incorporate linters is at the start of the +project. Relay did not, so adding these tools has been a slower, more +deliberate process. + +## Decision Drivers + +When picking a tool, there are some attributes to consider: + +- **Good defaults** - If a tool has good default settings, it is more likely + that it will need less project-specific configuration, and the project + code will look like other projects. +- **Mark false positives** - There should be few false positives, where the + tool identifies a problem but the code is OK. When there is a false + positive, it should be possible to ignore it and get a passing check. +- **Fixes issues when appropriate** - A linter that enforces layout is more + useful when it can re-write the source code to the correct layout. The + alternative, only identifying layout issues, would make for a worse + developer experience. In other cases, it is better to identify the issue and + let the developer decide how to fix it. +- **Checks on pull request** - The tool need to run in the continuous integration + (CI) environments, so that no failing code is merged to main. +- **Checks in the development environment** - It is easier to fix issues the + sooner they are found. It should be easy to run the tool in the development + environment. At a minimum, a developer should be able to run the tool once + per commit, and a clean linter run should (almost always) result in a clean + linter run in continuous integration. +- **Speed** - A faster tool will be run earlier in the development process. If + a tool runs in a millisecond, it can be incorporated into the editor, similar + to syntax highlighting. If a tool runs in 1/10 of a second, it can be run + every time the file is saved. If a tool takes a minute to run, developers + will disable it when committing and let CI run it. + +## Linters Used By Relay + +Relay has added several linters to check code quality: + +| | CSS | JavaScript | Python | Markdown | Shell Scripts | +| :--------------------- | :-------- | :--------------- | :----- | :------- | :------------ | +| Enforce Layout | prettier | prettier, ESLint | black | prettier | _none_ | +| Check Types | stylelint | TypeScript | mypy | _n/a_ | _none_ | +| Identify Mistakes | stylelint | ESLint | _none_ | _none_ | _none_ | +| Standardize Constructs | stylelint | ESLint | _none_ | _none_ | _none_ | +| Spelling and Grammar | _none_ | _none_ | _none_ | _none_ | _none_ | + +Relay includes [husky][] and [lint-staged][] to run linters as a pre-commit +hook. The linters configured in [.lintstagedrc.js][] run against the files +changed in each commit. When a tool supports fixing issues, it can update the +files before committing. Otherwise, detected issues will halt the commit, +giving the developer a chance to fix them. + +[husky]: https://typicode.github.io/husky/ +[lint-staged]: https://github.com/lint-staged/lint-staged +[.lintstagedrc.js]: https://github.com/mozilla/fx-private-relay/blob/main/.lintstagedrc.js + +### stylelint + +[stylelint][] is a CSS linter that identifies mistakes and standardizes +constructs. Relay uses a plugin to also check [SCSS/Sass][sass], an extended +syntax for CSS. The configuration file is at +[frontend/.stylelintrc.cjs][]. It was added July 2021. + +stylelint has all six attributes of a useful linter: + +- **Good defaults** - Stylelint has no linting rules enabled by default. However, + the team ships a minimum [recommended config][stylelint-config-recommended]. + They extend it with a [standard config][stylelint-config-standard] to enforce + common conventions. Relay uses [stylelint-config-recommended-scss][], and + extends it with some customer rules. +- **Mark False Positives** - Developers can + [disable a rule with a comment][stylelint-disable], and + [ignore a file in the config][stylelint-ignore]. +- **Fixes Issues when Appropriate** - The tool can fix some issues with the + `--fix` option. Auto-fix can be turned off for individual rules. +- **Checks on Pull Request** - The command-line tool is used to check all `.scss` + file for each pull request. +- **Checks in the Development Environment** - The tool is used in pre-commit + checks in auto-fix mode. There is an official stylelint plugin for the VS + Code editor, but it may duplicate or collide with the built-in CSS checks. +- **Speed** - The tool is fast enough to run on each file save. + +[sass]: https://sass-lang.com/documentation/syntax/ +[stylelint-config-recommended-scss]: https://github.com/stylelint-scss/stylelint-config-recommended-scss +[stylelint-config-recommended]: https://github.com/stylelint/stylelint-config-recommended +[stylelint-config-standard]: https://www.npmjs.com/package/stylelint-config-standard +[frontend/.stylelintrc.cjs]: https://github.com/mozilla/fx-private-relay/blob/main/frontend/.stylelintrc.cjs +[stylelint-disable]: https://github.com/mozilla/fx-private-relay/blob/6ca835d6ad69603fb80d23cdc12ba56aeeea1264/frontend/src/components/dashboard/aliases/Alias.module.scss#L215 +[stylelint-ignore]: https://github.com/mozilla/fx-private-relay/blob/6ca835d6ad69603fb80d23cdc12ba56aeeea1264/frontend/.stylelintrc.cjs#L13 +[stylelint]: https://stylelint.io/ + +### prettier + +[prettier][] is a code formatter for several languages. Relay uses it to format +JavaScript, TypeScript, CSS, SCSS, and Markdown (in the frontend folder). There +is an empty configuration file at [frontend/.prettierrc.json][]. It was added +(again?) in December 2021. + +prettier has all six attributes of a useful linter: + +- **Good defaults** - The default configuration is used on Relay. +- **Mark False Positives** - prettier supports an ignore file, but this is + unused on Relay. +- **Fixes Issues when Appropriate** - The `--write` option will reformat files, + and supports wildcard patterns. +- **Checks on Pull Request** - The tool is used to check all files in + `frontend/src` for each pull request. +- **Checks in the Development Environment** - The tool is used in pre-commit + checks in write mode. There is an official prettier plugin for VS Code and + other editors, and can be set to run on save. +- **Speed** - The tool is fast enough to run on each file save. It says it + may be slow to run across an entire project. + +[prettier]: https://prettier.io/ +[frontend/.prettierrc.json]: https://github.com/mozilla/fx-private-relay/blob/main/frontend/.prettierrc.json + +### TypeScript + +[TypeScript][] extends JavaScript with type information. This can be used for +static checking of code, including finding errors like unknown variables. It is +compiled to JavaScript in a build step, mostly by removing the type hints. Relay +added TypeScript in March 2022 with the refactor to React / next.js. + +TypeScript supports some of the attributes of a useful linter: + +- **Good defaults** - The configuration at [frontend/tsconfig.json][] has a few + customizations, many to work with next.js. Relay has enabled the strict mode, + which turns on a default set of rules. +- **Checks on Pull Request** - The frontend application is built on each pull + request, and fails on TypeScript errors. +- **Checks in the Development Environment** - A developer can run a service + that watches for file changes and rebuilds the application. The `next lint` + command runs during pre-commit and will check for TypeScript issues. +- **Speed** - The incremental build is fast enough to run on each file save. + +[TypeScript]: https://www.typescriptlang.org/ +[frontend/tsconfig.json]: https://github.com/mozilla/fx-private-relay/blame/main/frontend/tsconfig.json + +### next lint / ESLint + +Next.js [integrates with ESLint][nextjs-eslint]. It identifies mistakes and +standardizes constructs in JavaScript. Relay uses Next.js's base ESLint +configuration. A stricter rule set that checks against +[Google's Web Vitals][web-vitals] is also available. ESLint contains some +code formatting rules, and needs tuning to be compatible with prettier. +Relay has used ESLint since at least October 2020, and continued using it with +the next.js refactor in March 2022. + +ESLint has all six attributes of a useful linter: + +- **Good defaults** - Next.js provides a base and strict rule set. We've + expanded [frontend/.eslintrc.js][] to support Relay's non-standard + configuration (static HTML generation with a Django server rather than a + next.js server). +- **Mark False Positives** - The configuration allows tuning rules to work with + Relay's service architecture. ESLint does not have a way to ignore rules + within a source file. +- **Fixes Issues when Appropriate** - The `--fix` option will change code. +- **Checks on Pull Request** - `next lint` is used to check all the frontend + code for each pull request. +- **Checks in the Development Environment** - The tool is used in pre-commit + checks in fix mode. There are editor integrations several editors including + VS Code. +- **Speed** - The tool is fast enough to run on each file save. + +[nextjs-eslint]: https://nextjs.org/docs/pages/building-your-application/configuring/eslint +[web-vitals]: https://web.dev/articles/vitals +[frontend/.eslintrc.js]: https://github.com/mozilla/fx-private-relay/blob/main/frontend/.eslintrc.js + +### black + +[black][] is a formatting tool to enforce layout of Python code. Relay adopted +it in May 2022. + +black has all six attributes of a useful linter: + +- **Good defaults** - black started with almost no options. As it has grown and + added maintainers, it is now more flexible about formatting code and + targeting a minimum Python version. It uses the standard [pyproject.toml][] + for configuration. Relay uses the default configuration. +- **Mark False Positives** - black allows `# fmt` comments to ignore formatting + for a section of code. Relay does not use this. +- **Fixes Issues when Appropriate** - The default is to reformat code, and + report when a file changed due to reformatting +- **Checks on Pull Request** - `black --check` is used to check Python code + formatting for each pull request. +- **Checks in the Development Environment** - The tool is used in pre-commit + checks. It can be integrated into editors, including VS Code, to reformat + the file on save. +- **Speed** - The tool is fast enough to run on each file save, as well as + periodically against the entire codebase. + +[black]: https://black.readthedocs.io/en/stable/index.html +[pyproject.toml]: https://github.com/mozilla/fx-private-relay/blob/main/pyproject.toml + +### mypy + +[mypy][] is a static type checker for Python. It can be used in strict mode, +helping a new project develop type-checked code. Relay added mypy support in +April 2022, using an optional checking configuration as appropriate for a +legacy project. The Relay team has added type hints to new code and gradually +ratcheted up the strictness. + +mypy has some of the attributes of a useful linter: + +- **Good defaults** - In strict mode, mypy will check types for all Python + code. This can be challenging when working with legacy code or third-party + libraries that do not ship type hints. The project has a guide + [Using mypy with an existing codebase][mypy-legacy] which Relay used to guide + the implementation. +- **Mark False Positives** - mypy allows ignoring third-party packages in + the standard [pyproject.toml][] file. Relay uses this for + third-party packages without type hints. Relay also disables strict rules + that require many changes to legacy code. A comment `# type: ignore` can + be used to ignore issues in code. The comment feature is not used by Relay. +- **Checks on Pull Request** - mypy is used to check Python types for each pull + request. +- **Checks in the Development Environment** - The tool is used in pre-commit + checks. It can be integrated into editors, including VS Code, to identify + issues on save. +- **Speed** - The tool maintains a cache to make incremental type checking + faster. It is occasionally necessary to use the `--no-incremental` option + to detect issues that cross source files. + +[mypy]: https://mypy.readthedocs.io/en/stable/ +[mypy-legacy]: https://mypy.readthedocs.io/en/stable/existing_code.html + +## Adding New Linters + +Here is the linter chart again, focusing on areas without linters: + +| | CSS | JavaScript | Python | Markdown | Shell Scripts | +| :--------------------- | :----- | :--------- | :----- | :------- | :------------ | +| Enforce Layout | yes | yes | yes | yes | **no** | +| Check Types | yes | yes | yes | n/a | **no** | +| Identify Mistakes | yes | yes | **no** | **no** | **no** | +| Standardize Constructs | yes | yes | **no** | **no** | **no** | +| Spelling and Grammar | **no** | **no** | **no** | **no** | **no** | + +When deciding _if a linter is needed_, some criteria are: + +- **Is the language used in production?** CSS, JavaScript, and Python are used + in production. It is useful to find issues in this code before it is shipped + to users. Markdown and Shell Scripts are used by developers, not users, so + errors do not impact users directly, or at all. +- **What is the impact of issues detected by the linter?** The highest impact + issues are user-facing bugs, and some tools address this directly. Often a + linter's largest impact is to make code more uniform, which has a measurable + impact on the speed and effectiveness of code reviews. Reviewers can spend + time understanding the logic of code changes without the distraction of + parsing non-standard code. A good code comment with a spelling error can + still be understood by another developer. A perfectly formed but misleading + comment should be deleted. + +By these tests, we should not add new linters for Markdown and Shell Scripts +at this time, or add Spelling and Grammar linters. Individuals can use these +tools in development and code review, but it is not worth enforcing a tool +for all new code. + +On the other hand, we should consider a Python linter that will identify +mistakes and standardize constructs. Python is used in production for the +web and API server, as well as background tasks. Errors in this code will +impact users, and non-standard code can slow down code reviews. + +This document proposes six criteria for evaluating a linter. Some linters +that would fill this role: + +- [pylint][] - This tool has been in development since 2003, and has + hundreds of built-in checks, including layout enforcement and spelling + checkers. It works across entire codebases instead of individual files. It + identifies 2,800 issues in our code, in 25 seconds. +- [pycodestyle][] - This tool, formerly known as `pep8`, has been in + development since 2006, and is focused on code formatting and some code + constructs. It identifies 317 issues in our code in 1.5 seconds. +- [pyflakes][] - This tool has been in development since 2009, targeted + at identifying mistakes in code without caring about formatting. It finds 12 + issues in our code in 12 seconds. +- [flake8][] - This tool has been in development since 2010, and combines + pycodestyle, pyflakes and the `mccabe` tool into one package. It also + has a plugin system that can extend the ruleset. It identifies 277 issues in + our code in 600 milliseconds. +- [isort][] - This tool has been in development since 2017, and reformats + imports to enforce a sort order and import style. It detects 99 issues in + our code in 750 milliseconds. +- [bandit][] - This tool scans code for security issues. It finds 1997 low + severity and 8 medium severity issues in our code in 1.5 seconds. +- [ruff][] - This tool, launched by VC-backed [Astral][] in 2023 and written in + Rust, is optimized for speed and automatic fixing. With the default rules, + which implement some `flake8` rules, it identifies 26 issues in our code, + 9 fixable, in 35 milliseconds. It has optional rules that implement the + checks of all the previous tools. + +[pylint]: https://pylint.readthedocs.io +[ruff]: https://docs.astral.sh/ruff/ +[Astral]: https://astral.sh/blog/announcing-astral-the-company-behind-ruff +[flake8]: https://flake8.pycqa.org/en/latest/index.html +[pycodestyle]: https://pycodestyle.pycqa.org/en/latest/ +[pyflakes]: https://github.com/PyCQA/pyflakes +[isort]: https://pycqa.github.io/isort/index.html +[bandit]: https://github.com/PyCQA/bandit + +`ruff` is a good choice to add missing coverage for Python. It fulfills +all six criteria, and excels at speed. + +- **Good defaults** - It provides similar checks to `flake8` by default. + The default configuration is compatible with `black` and Python 3.8. + It uses the standard [pyproject.toml][] for configuration. It provides + rulesets that emulate `pylint`, `pycodestyle`, `isort`, and `bandit`. + It also provides rules for dozens of popular `flake8` plugins. +- **Mark False Positives** - `ruff` supports exception rules in the + configuration file. It supports the comment `# ruff: noqa` to turn off + errors in code. It also checks for instances where the comment is not + needed. +- **Fixes Issues when Appropriate** - The `ruff check --fix` option will change + code when possible. +- **Checks on Pull Request** - `ruff check` can check Python code in pull + requests. +- **Checks in the Development Environment** - The tool can be used in + pre-commit checks in fix mode. There are editor integrations several editors + including VS Code. +- **Speed** - The tool is 10x to 100x faster than other tools, fast enough + to run on each file save. + +The largest negative for `ruff` is that Astral is a VC-supported company, and +the tool may undergo changes in the future for monetization. It may be useful +to run the other tools supported by [PyCQA][] in parallel and in continuous +integration. This would ensure a community-supported alternative is available, +and may expose a different set of issues. + +[PyCQA]: https://github.com/PyCQA From c1792653e48d70e8379f689dabded5508c584d4b Mon Sep 17 00:00:00 2001 From: John Whitlock Date: Thu, 4 Apr 2024 13:46:07 -0500 Subject: [PATCH 2/6] second draft --- docs/adr/0004-use-code-linters.md | 406 +++++++++++++----------------- 1 file changed, 175 insertions(+), 231 deletions(-) diff --git a/docs/adr/0004-use-code-linters.md b/docs/adr/0004-use-code-linters.md index 62e5fd327a..45424c29ca 100644 --- a/docs/adr/0004-use-code-linters.md +++ b/docs/adr/0004-use-code-linters.md @@ -10,8 +10,9 @@ Technical Story: [MPP-79][] ## Context and Problem Statement -A linter is a static analysis tool that find bugs, style issues, and suspicious -constructs. Some of the areas addressed by different linting tools include: +A linter is a static analysis tool that finds bugs, style issues, and +suspicious constructs. Some of the areas addressed by different linting tools +include: - **Enforce Layout** - Checks (and often reformats) code to fit a project style. This helps make code and code changes easier to read and comprehend. @@ -23,11 +24,11 @@ constructs. Some of the areas addressed by different linting tools include: programming mistake. Some examples are identifying unused variables, and using loose equality in JavaScript. - **Standardize Constructs** - Picks a way to express a concept, when the - language allows several was to express it. Some examples are import order, + language allows several ways to express it. Some examples are import order, or if default values should be specified by the caller. -- **Spelling and Grammar Checking** - Checks comments in a similar way to a - word processor. Comments often use project-specific technical terms, making - this a different task from checking English prose. +- **Spelling and Grammar Checking** - Checks prose and comments in a similar + way to a word processor. Comments often use project-specific technical + terms, making this a different task from checking English prose. Most developers appreciate that linters make multi-developer projects easier. The shared project standards can be checked and enforced by tools. Code @@ -36,41 +37,13 @@ A linter may identify an unexpected issue. A developer can read the linter documentation to learn more about the issue. This is one way a developer can improve their understanding of the project's languages. -On the other hand, developers resent changes to their working process, +On the other hand, developers are annoyed by changes to their working process, especially when the benefits are unclear or they disagree with the trade-offs. It becomes harder to add a tool as the lines of code and the number of team members grow. The best time to incorporate linters is at the start of the project. Relay did not, so adding these tools has been a slower, more deliberate process. -## Decision Drivers - -When picking a tool, there are some attributes to consider: - -- **Good defaults** - If a tool has good default settings, it is more likely - that it will need less project-specific configuration, and the project - code will look like other projects. -- **Mark false positives** - There should be few false positives, where the - tool identifies a problem but the code is OK. When there is a false - positive, it should be possible to ignore it and get a passing check. -- **Fixes issues when appropriate** - A linter that enforces layout is more - useful when it can re-write the source code to the correct layout. The - alternative, only identifying layout issues, would make for a worse - developer experience. In other cases, it is better to identify the issue and - let the developer decide how to fix it. -- **Checks on pull request** - The tool need to run in the continuous integration - (CI) environments, so that no failing code is merged to main. -- **Checks in the development environment** - It is easier to fix issues the - sooner they are found. It should be easy to run the tool in the development - environment. At a minimum, a developer should be able to run the tool once - per commit, and a clean linter run should (almost always) result in a clean - linter run in continuous integration. -- **Speed** - A faster tool will be run earlier in the development process. If - a tool runs in a millisecond, it can be incorporated into the editor, similar - to syntax highlighting. If a tool runs in 1/10 of a second, it can be run - every time the file is saved. If a tool takes a minute to run, developers - will disable it when committing and let CI run it. - ## Linters Used By Relay Relay has added several linters to check code quality: @@ -84,9 +57,9 @@ Relay has added several linters to check code quality: | Spelling and Grammar | _none_ | _none_ | _none_ | _none_ | _none_ | Relay includes [husky][] and [lint-staged][] to run linters as a pre-commit -hook. The linters configured in [.lintstagedrc.js][] run against the files -changed in each commit. When a tool supports fixing issues, it can update the -files before committing. Otherwise, detected issues will halt the commit, +hook. The linters are configured in [.lintstagedrc.js][]. They run against the +files changed in each commit. When a tool supports fixing issues, it can update +the files before committing. Otherwise, detected issues will halt the commit, giving the developer a chance to fix them. [husky]: https://typicode.github.io/husky/ @@ -95,60 +68,28 @@ giving the developer a chance to fix them. ### stylelint -[stylelint][] is a CSS linter that identifies mistakes and standardizes -constructs. Relay uses a plugin to also check [SCSS/Sass][sass], an extended -syntax for CSS. The configuration file is at -[frontend/.stylelintrc.cjs][]. It was added July 2021. - -stylelint has all six attributes of a useful linter: - -- **Good defaults** - Stylelint has no linting rules enabled by default. However, - the team ships a minimum [recommended config][stylelint-config-recommended]. - They extend it with a [standard config][stylelint-config-standard] to enforce - common conventions. Relay uses [stylelint-config-recommended-scss][], and - extends it with some customer rules. -- **Mark False Positives** - Developers can - [disable a rule with a comment][stylelint-disable], and - [ignore a file in the config][stylelint-ignore]. -- **Fixes Issues when Appropriate** - The tool can fix some issues with the - `--fix` option. Auto-fix can be turned off for individual rules. -- **Checks on Pull Request** - The command-line tool is used to check all `.scss` - file for each pull request. -- **Checks in the Development Environment** - The tool is used in pre-commit - checks in auto-fix mode. There is an official stylelint plugin for the VS - Code editor, but it may duplicate or collide with the built-in CSS checks. -- **Speed** - The tool is fast enough to run on each file save. +[stylelint][] identifies mistakes and standardizes constructs in CSS. Relay +uses a plugin to also check [SCSS/Sass][sass], an extended syntax for CSS. It +was added in July 2021. + +The configuration file is at [frontend/.stylelintrc.cjs][]. Relay uses the +[stylelint-config-recommended-scss][] ruleset, and extends it with custom +rules. [sass]: https://sass-lang.com/documentation/syntax/ [stylelint-config-recommended-scss]: https://github.com/stylelint-scss/stylelint-config-recommended-scss -[stylelint-config-recommended]: https://github.com/stylelint/stylelint-config-recommended -[stylelint-config-standard]: https://www.npmjs.com/package/stylelint-config-standard [frontend/.stylelintrc.cjs]: https://github.com/mozilla/fx-private-relay/blob/main/frontend/.stylelintrc.cjs -[stylelint-disable]: https://github.com/mozilla/fx-private-relay/blob/6ca835d6ad69603fb80d23cdc12ba56aeeea1264/frontend/src/components/dashboard/aliases/Alias.module.scss#L215 -[stylelint-ignore]: https://github.com/mozilla/fx-private-relay/blob/6ca835d6ad69603fb80d23cdc12ba56aeeea1264/frontend/.stylelintrc.cjs#L13 [stylelint]: https://stylelint.io/ ### prettier [prettier][] is a code formatter for several languages. Relay uses it to format -JavaScript, TypeScript, CSS, SCSS, and Markdown (in the frontend folder). There -is an empty configuration file at [frontend/.prettierrc.json][]. It was added -(again?) in December 2021. - -prettier has all six attributes of a useful linter: - -- **Good defaults** - The default configuration is used on Relay. -- **Mark False Positives** - prettier supports an ignore file, but this is - unused on Relay. -- **Fixes Issues when Appropriate** - The `--write` option will reformat files, - and supports wildcard patterns. -- **Checks on Pull Request** - The tool is used to check all files in - `frontend/src` for each pull request. -- **Checks in the Development Environment** - The tool is used in pre-commit - checks in write mode. There is an official prettier plugin for VS Code and - other editors, and can be set to run on save. -- **Speed** - The tool is fast enough to run on each file save. It says it - may be slow to run across an entire project. +JavaScript, TypeScript, CSS, SCSS, and Markdown (in the frontend folder). It +was added (again?) in December 2021. + +The (empty) configuration file is at [frontend/.prettierrc.json][]. Relay and +other projects use the `prettier` defaults, and many tools are compatible with +`prettier` by default or by configuration. [prettier]: https://prettier.io/ [frontend/.prettierrc.json]: https://github.com/mozilla/fx-private-relay/blob/main/frontend/.prettierrc.json @@ -160,17 +101,10 @@ static checking of code, including finding errors like unknown variables. It is compiled to JavaScript in a build step, mostly by removing the type hints. Relay added TypeScript in March 2022 with the refactor to React / next.js. -TypeScript supports some of the attributes of a useful linter: - -- **Good defaults** - The configuration at [frontend/tsconfig.json][] has a few - customizations, many to work with next.js. Relay has enabled the strict mode, - which turns on a default set of rules. -- **Checks on Pull Request** - The frontend application is built on each pull - request, and fails on TypeScript errors. -- **Checks in the Development Environment** - A developer can run a service - that watches for file changes and rebuilds the application. The `next lint` - command runs during pre-commit and will check for TypeScript issues. -- **Speed** - The incremental build is fast enough to run on each file save. +The configuration at [frontend/tsconfig.json][] has a few customizations, many +to work with next.js. Relay has enabled the strict mode, which turns on a +default set of rules. The types are checked during the frontend build and +with `next lint`. [TypeScript]: https://www.typescriptlang.org/ [frontend/tsconfig.json]: https://github.com/mozilla/fx-private-relay/blame/main/frontend/tsconfig.json @@ -178,131 +112,148 @@ TypeScript supports some of the attributes of a useful linter: ### next lint / ESLint Next.js [integrates with ESLint][nextjs-eslint]. It identifies mistakes and -standardizes constructs in JavaScript. Relay uses Next.js's base ESLint -configuration. A stricter rule set that checks against -[Google's Web Vitals][web-vitals] is also available. ESLint contains some -code formatting rules, and needs tuning to be compatible with prettier. +standardizes constructs in JavaScript. It can fix some issues automatically. Relay has used ESLint since at least October 2020, and continued using it with the next.js refactor in March 2022. -ESLint has all six attributes of a useful linter: - -- **Good defaults** - Next.js provides a base and strict rule set. We've - expanded [frontend/.eslintrc.js][] to support Relay's non-standard - configuration (static HTML generation with a Django server rather than a - next.js server). -- **Mark False Positives** - The configuration allows tuning rules to work with - Relay's service architecture. ESLint does not have a way to ignore rules - within a source file. -- **Fixes Issues when Appropriate** - The `--fix` option will change code. -- **Checks on Pull Request** - `next lint` is used to check all the frontend - code for each pull request. -- **Checks in the Development Environment** - The tool is used in pre-commit - checks in fix mode. There are editor integrations several editors including - VS Code. -- **Speed** - The tool is fast enough to run on each file save. +Relay uses Next.js's base ESLint configuration. This includes the tuning needed +to make [ESLint][] formatting compatible with `prettier`. A stricter rule set +that checks against [Google's Web Vitals][web-vitals] is also available. Relay +has expanded [frontend/.eslintrc.js][] to support our non-standard +configuration (static HTML generation with a Django server rather than a +next.js server). +[ESLint]: https://eslint.org/docs/latest/ [nextjs-eslint]: https://nextjs.org/docs/pages/building-your-application/configuring/eslint [web-vitals]: https://web.dev/articles/vitals [frontend/.eslintrc.js]: https://github.com/mozilla/fx-private-relay/blob/main/frontend/.eslintrc.js ### black -[black][] is a formatting tool to enforce layout of Python code. Relay adopted -it in May 2022. - -black has all six attributes of a useful linter: - -- **Good defaults** - black started with almost no options. As it has grown and - added maintainers, it is now more flexible about formatting code and - targeting a minimum Python version. It uses the standard [pyproject.toml][] - for configuration. Relay uses the default configuration. -- **Mark False Positives** - black allows `# fmt` comments to ignore formatting - for a section of code. Relay does not use this. -- **Fixes Issues when Appropriate** - The default is to reformat code, and - report when a file changed due to reformatting -- **Checks on Pull Request** - `black --check` is used to check Python code - formatting for each pull request. -- **Checks in the Development Environment** - The tool is used in pre-commit - checks. It can be integrated into editors, including VS Code, to reformat - the file on save. -- **Speed** - The tool is fast enough to run on each file save, as well as - periodically against the entire codebase. +[black][] is a formatting tool to enforce layout of Python code. It rewrites +files in place, and reports if any files were reformatted. Relay adopted it in +May 2022. + +`black` uses [pyproject.toml][] for configuration. Relay uses the default +configuration, but may add the minimum expected Python version to allow +using more recent syntax. [black]: https://black.readthedocs.io/en/stable/index.html [pyproject.toml]: https://github.com/mozilla/fx-private-relay/blob/main/pyproject.toml ### mypy -[mypy][] is a static type checker for Python. It can be used in strict mode, -helping a new project develop type-checked code. Relay added mypy support in -April 2022, using an optional checking configuration as appropriate for a -legacy project. The Relay team has added type hints to new code and gradually -ratcheted up the strictness. - -mypy has some of the attributes of a useful linter: - -- **Good defaults** - In strict mode, mypy will check types for all Python - code. This can be challenging when working with legacy code or third-party - libraries that do not ship type hints. The project has a guide - [Using mypy with an existing codebase][mypy-legacy] which Relay used to guide - the implementation. -- **Mark False Positives** - mypy allows ignoring third-party packages in - the standard [pyproject.toml][] file. Relay uses this for - third-party packages without type hints. Relay also disables strict rules - that require many changes to legacy code. A comment `# type: ignore` can - be used to ignore issues in code. The comment feature is not used by Relay. -- **Checks on Pull Request** - mypy is used to check Python types for each pull - request. -- **Checks in the Development Environment** - The tool is used in pre-commit - checks. It can be integrated into editors, including VS Code, to identify - issues on save. -- **Speed** - The tool maintains a cache to make incremental type checking - faster. It is occasionally necessary to use the `--no-incremental` option - to detect issues that cross source files. +[mypy][] is a static type checker for Python. Relay added mypy support in April +2022, using an optional checking configuration as appropriate for an +[existing codebase][mypy-legacy]. + +`mypy` uses [pyproject.toml][] for configuration. Relay ignores issues with +third party libraries that do not ship type hints. Relay disables strict rules +that require code changes. These changes are made incrementally as technical +debt projects. [mypy]: https://mypy.readthedocs.io/en/stable/ [mypy-legacy]: https://mypy.readthedocs.io/en/stable/existing_code.html -## Adding New Linters +## Adding A New Linter -Here is the linter chart again, focusing on areas without linters: +Adding a new linter is not free. Each developer needs to add it to their workflow. +Continuous Integration needs to run the linter on each pull request. Linting +issues must be addressed before merging. -| | CSS | JavaScript | Python | Markdown | Shell Scripts | -| :--------------------- | :----- | :--------- | :----- | :------- | :------------ | -| Enforce Layout | yes | yes | yes | yes | **no** | -| Check Types | yes | yes | yes | n/a | **no** | -| Identify Mistakes | yes | yes | **no** | **no** | **no** | -| Standardize Constructs | yes | yes | **no** | **no** | **no** | -| Spelling and Grammar | **no** | **no** | **no** | **no** | **no** | +The decision to add a linter has two parts. First, is a linter needed? Second, +which linter should be added? + +### Decision Drivers: Is a Linter Needed? When deciding _if a linter is needed_, some criteria are: -- **Is the language used in production?** CSS, JavaScript, and Python are used - in production. It is useful to find issues in this code before it is shipped - to users. Markdown and Shell Scripts are used by developers, not users, so - errors do not impact users directly, or at all. +- **Is the language used in production?** It is useful to find issues in this + code before it is shipped to users. - **What is the impact of issues detected by the linter?** The highest impact issues are user-facing bugs, and some tools address this directly. Often a - linter's largest impact is to make code more uniform, which has a measurable + linter's purpose is to make code more uniform, which has a measurable impact on the speed and effectiveness of code reviews. Reviewers can spend time understanding the logic of code changes without the distraction of - parsing non-standard code. A good code comment with a spelling error can - still be understood by another developer. A perfectly formed but misleading - comment should be deleted. + parsing. -By these tests, we should not add new linters for Markdown and Shell Scripts -at this time, or add Spelling and Grammar linters. Individuals can use these -tools in development and code review, but it is not worth enforcing a tool -for all new code. +Here is the linter chart again, focusing on areas without linters: -On the other hand, we should consider a Python linter that will identify -mistakes and standardize constructs. Python is used in production for the -web and API server, as well as background tasks. Errors in this code will -impact users, and non-standard code can slow down code reviews. +| | CSS | JavaScript | Python | Markdown | Shell Scripts | +| :--------------------- | :-- | :--------- | :----- | :------- | :------------ | +| Enforce Layout | yes | yes | yes | yes | no | +| Check Types | yes | yes | yes | n/a | no | +| Identify Mistakes | yes | yes | **no** | no | no | +| Standardize Constructs | yes | yes | **no** | no | no | +| Spelling and Grammar | no | no | no | no | no | + +Relay does not need more linters around Markdown at this time. Our +documentation is developer-facing, not user-facing. Errors in these documents +can be fixed on first review, and as they are encountered. There are tools like +the [markdownlint demo][] that can be used as needed. + +Relay does not need shell script linters at this time. These scripts are used +in CircleCI to automate build and test steps. Errors in these scripts appear as +broken builds, and are fixed as part of normal development. They do not have a +direct impact on our users. There are tools like [ShellCheck][] that can be +used as needed. + +Relay does not need linters around spelling and grammar at this time. +User-facing strings are translated, and are reviewed closely. A good code +comment with a spelling error can still be understood by another developer. A +perfectly formed but misleading comment should be deleted. There are tools +like [PyEnchant][], [Hemmingway][], and word processors that can be used by +developers without being checked in continuous integration. + +As highlighted, there is a gap in Python linters. There is no official linter +to identify mistakes and standardize constructs. Python is used in production +for the web and API server, as well as background tasks. Errors in this code +will impact users, and non-standard code can slow down code reviews. This gap +meets the criteria for adding a new linter to CI and development. + +[markdownlint demo]: https://dlaa.me/markdownlint/ +[ShellCheck]: https://www.shellcheck.net/ +[PyEnchant]: https://pyenchant.github.io/pyenchant/# +[Hemingway]: https://hemingwayapp.com/ + +## Decision Drivers for Choosing Between Similar Linters + +A linter that is required for Relay development must have these attributes: + +- **Checks on pull request** - The tool needs to run in the continuous + integration (CI) environments, so that no failing code is merged to main. +- **Runs in the development environment** - Developers need to be able to + run the tool locally. At a minimum, a developer should be able to run the + tool once per commit. The developer should be able to run the tool manually + as well. A developer should have high confidence that the tool runs the + same locally as in CI. +- **Marks false positives** - There should be few false positives, where the + tool identifies a problem but the code is OK. When there is a false + positive, it should be possible to ignore it and get a passing check. -This document proposes six criteria for evaluating a linter. Some linters -that would fill this role: +When choosing between similar tools, these attributes can help guide the +decision: + +- **Good defaults** - If a tool has good default settings, it is more likely + that it will need less project-specific configuration. Developers can use + tips and tricks from other projects using the tool. +- **Fixes issues when appropriate** - A tool that enforces layout is more + useful when it can rewrite the source code to the correct layout. The + alternative, only identifying layout issues, makes for a worse developer + experience. In other cases, it is better to identify the issue and let the + developer decide how to fix it. +- **Editor integration** - When a tool is incorporated in the developer's code + editor, enforcing layout and fixing issues becomes a natural part of the + code writing process. +- **Speed** - A faster tool will be run earlier in the development process. If + a tool runs in a millisecond, it can be incorporated into the editor, + similar to syntax highlighting. If a tool runs in 100 milliseconds, it can + be run every time the file is saved. If a tool takes a minute to run, + developers will avoid running it locally, and wait for CI to identify + issues. + +There are many tools that address identifying mistakes and standardizing +constructs in Python. Some linters that would fill this role: - [pylint][] - This tool has been in development since 2003, and has hundreds of built-in checks, including layout enforcement and spelling @@ -312,58 +263,51 @@ that would fill this role: development since 2006, and is focused on code formatting and some code constructs. It identifies 317 issues in our code in 1.5 seconds. - [pyflakes][] - This tool has been in development since 2009, targeted - at identifying mistakes in code without caring about formatting. It finds 12 - issues in our code in 12 seconds. + at identifying mistakes in code without caring about formatting. It finds + 12 issues in our code in 12 seconds. - [flake8][] - This tool has been in development since 2010, and combines - pycodestyle, pyflakes and the `mccabe` tool into one package. It also - has a plugin system that can extend the ruleset. It identifies 277 issues in - our code in 600 milliseconds. + `pycodestyle`, `pyflakes` and the `mccabe` tool into one package. It also + has a plugin system that can extend the ruleset. It identifies 277 issues + in our code in 600 milliseconds. - [isort][] - This tool has been in development since 2017, and reformats imports to enforce a sort order and import style. It detects 99 issues in - our code in 750 milliseconds. + our code in 750 milliseconds. It can be used as a `flake8` plugin. - [bandit][] - This tool scans code for security issues. It finds 1997 low - severity and 8 medium severity issues in our code in 1.5 seconds. -- [ruff][] - This tool, launched by VC-backed [Astral][] in 2023 and written in + severity and 8 medium severity issues in our code in 1.5 seconds. It can be + run as a `flake8` plugin. +- [ruff][] - This tool, launched in 2023 and written in Rust, is optimized for speed and automatic fixing. With the default rules, - which implement some `flake8` rules, it identifies 26 issues in our code, - 9 fixable, in 35 milliseconds. It has optional rules that implement the + which implement some `flake8` rules, it identifies 26 issues in our code, 9 + fixable, in 35 milliseconds. It has optional rules that implement the checks of all the previous tools. -[pylint]: https://pylint.readthedocs.io -[ruff]: https://docs.astral.sh/ruff/ +All the tools have the required attributes. They can be run in CI and in +the development environment. They are configurable, and have a mechanism for +marking false positives. + +Three approaches that stand out: + +- `pylint` for identifying the most issues. The defaults will need to be + tuned to remove false positives, such as warnings on `assert` in tests. It + is slow enough to cause pain if used for local development. +- `flake8` with several plugins (`isort`, `bandit`, others). This is a + good mix of speed and coverage. The tools and plugins are maintained by + [PyCQA][] and integrate well together. +- `ruff` with additional checks enabled. This tool can perform many of the + checks that the other tools check. It runs 10x - 100x faster than the + other tools. It also is compatible with `black` formatting by default. + The largest negative is that [Astral][], the company that develops + `ruff`, is VC-backed, and the tool may add monetization in the future. + +Due to the speed and flexibility, `ruff` is the recommended tool for the +next linter, covering the important linting gap for Python. + [Astral]: https://astral.sh/blog/announcing-astral-the-company-behind-ruff +[PyCQA]: https://github.com/PyCQA +[bandit]: https://github.com/PyCQA/bandit [flake8]: https://flake8.pycqa.org/en/latest/index.html +[isort]: https://pycqa.github.io/isort/index.html [pycodestyle]: https://pycodestyle.pycqa.org/en/latest/ [pyflakes]: https://github.com/PyCQA/pyflakes -[isort]: https://pycqa.github.io/isort/index.html -[bandit]: https://github.com/PyCQA/bandit - -`ruff` is a good choice to add missing coverage for Python. It fulfills -all six criteria, and excels at speed. - -- **Good defaults** - It provides similar checks to `flake8` by default. - The default configuration is compatible with `black` and Python 3.8. - It uses the standard [pyproject.toml][] for configuration. It provides - rulesets that emulate `pylint`, `pycodestyle`, `isort`, and `bandit`. - It also provides rules for dozens of popular `flake8` plugins. -- **Mark False Positives** - `ruff` supports exception rules in the - configuration file. It supports the comment `# ruff: noqa` to turn off - errors in code. It also checks for instances where the comment is not - needed. -- **Fixes Issues when Appropriate** - The `ruff check --fix` option will change - code when possible. -- **Checks on Pull Request** - `ruff check` can check Python code in pull - requests. -- **Checks in the Development Environment** - The tool can be used in - pre-commit checks in fix mode. There are editor integrations several editors - including VS Code. -- **Speed** - The tool is 10x to 100x faster than other tools, fast enough - to run on each file save. - -The largest negative for `ruff` is that Astral is a VC-supported company, and -the tool may undergo changes in the future for monetization. It may be useful -to run the other tools supported by [PyCQA][] in parallel and in continuous -integration. This would ensure a community-supported alternative is available, -and may expose a different set of issues. - -[PyCQA]: https://github.com/PyCQA +[pylint]: https://pylint.readthedocs.io +[ruff]: https://docs.astral.sh/ruff/ From 30f4d7b64e63a77415f6f554b13cc6081bbf9aee Mon Sep 17 00:00:00 2001 From: John Whitlock Date: Thu, 4 Apr 2024 15:49:20 -0500 Subject: [PATCH 3/6] third draft --- docs/adr/0004-use-code-linters.md | 253 ++++++++++++++---------------- 1 file changed, 120 insertions(+), 133 deletions(-) diff --git a/docs/adr/0004-use-code-linters.md b/docs/adr/0004-use-code-linters.md index 45424c29ca..4669b5ad5f 100644 --- a/docs/adr/0004-use-code-linters.md +++ b/docs/adr/0004-use-code-linters.md @@ -10,39 +10,39 @@ Technical Story: [MPP-79][] ## Context and Problem Statement -A linter is a static analysis tool that finds bugs, style issues, and -suspicious constructs. Some of the areas addressed by different linting tools -include: +A linter is a static analysis tool that aids development. Some of the areas +addressed by different linting tools include: -- **Enforce Layout** - Checks (and often reformats) code to fit a project +- **Enforce Layout**: A tool can check and even reformat code to fit a project style. This helps make code and code changes easier to read and comprehend. It also allows new contributors to match the existing project style. -- **Check Types** - Verifies that code is using types consistently. - This is especially useful for dynamic languages where incorrect type errors - would lead to bugs or runtime exceptions. -- **Identify Mistakes** - Detects valid code that is often associated with a - programming mistake. Some examples are identifying unused variables, and - using loose equality in JavaScript. -- **Standardize Constructs** - Picks a way to express a concept, when the - language allows several ways to express it. Some examples are import order, - or if default values should be specified by the caller. -- **Spelling and Grammar Checking** - Checks prose and comments in a similar - way to a word processor. Comments often use project-specific technical - terms, making this a different task from checking English prose. +- **Check Types**: Dynamic languages are flexible about variable types. + Valid code with incorrect types can result in bugs and runtime errors. A + linter can check type hints for consistent usage. Code editors can use type + hints for documentation and assisted code writing. +- **Identify Mistakes**: A tool can detect some common mistakes. Examples are + identifying unused variables, and using loose equality in JavaScript. +- **Standardize Constructs**: There are many ways to express the same logic in + code. A linter can suggest or rewrite code to a standard form. One example is + the order and placement of imports. Another is omitting optional defaults to + a function call. +- **Spelling and Grammar Checking**: A tool can check for misspelling and grammar + mistakes. The tools have different priorities from similar tools for word + processors. The text can appear in prose and code comments. The text includes + technical and project-specific terms. Most developers appreciate that linters make multi-developer projects easier. -The shared project standards can be checked and enforced by tools. Code -reviewers can focus on the logic of changes, rather than the form of the code. -A linter may identify an unexpected issue. A developer can read the linter -documentation to learn more about the issue. This is one way a developer can -improve their understanding of the project's languages. - -On the other hand, developers are annoyed by changes to their working process, -especially when the benefits are unclear or they disagree with the trade-offs. -It becomes harder to add a tool as the lines of code and the number of team -members grow. The best time to incorporate linters is at the start of the -project. Relay did not, so adding these tools has been a slower, more -deliberate process. +Tool check and enforce project standards. Code reviewers focus on the logic of +changes, rather than the form of the code. A linter may identify an unexpected +issue. A developer can read the linter documentation to learn more about the +issue. This is one way a developer can improve their understanding of the +project's languages. + +Also, developers dislike changes to their working process. The benefits may be +unclear. They may disagree that the cost is worth the benefits. It becomes +harder to add a tool as the lines of code and the number of team members grow. +The best time to incorporate linters is at the start of the project. Relay did +not, so adding these tools has been a slower, more deliberate process. ## Linters Used By Relay @@ -69,8 +69,8 @@ giving the developer a chance to fix them. ### stylelint [stylelint][] identifies mistakes and standardizes constructs in CSS. Relay -uses a plugin to also check [SCSS/Sass][sass], an extended syntax for CSS. It -was added in July 2021. +uses a plugin to also check [SCSS/Sass][sass], an extended syntax for CSS. +Relay added `stylelint` in July 2021. The configuration file is at [frontend/.stylelintrc.cjs][]. Relay uses the [stylelint-config-recommended-scss][] ruleset, and extends it with custom @@ -84,11 +84,11 @@ rules. ### prettier [prettier][] is a code formatter for several languages. Relay uses it to format -JavaScript, TypeScript, CSS, SCSS, and Markdown (in the frontend folder). It -was added (again?) in December 2021. +JavaScript, TypeScript, CSS, SCSS, and Markdown. Relay added `prettier` (again?) +in December 2021. The (empty) configuration file is at [frontend/.prettierrc.json][]. Relay and -other projects use the `prettier` defaults, and many tools are compatible with +other projects use the `prettier` defaults. Many tools are compatible with `prettier` by default or by configuration. [prettier]: https://prettier.io/ @@ -96,15 +96,14 @@ other projects use the `prettier` defaults, and many tools are compatible with ### TypeScript -[TypeScript][] extends JavaScript with type information. This can be used for -static checking of code, including finding errors like unknown variables. It is -compiled to JavaScript in a build step, mostly by removing the type hints. Relay -added TypeScript in March 2022 with the refactor to React / next.js. +[TypeScript][] extends JavaScript with type information. It converts to +JavaScript in a build step, mostly by removing the type hints. Relay added +TypeScript in March 2022 with the refactor to React / next.js. -The configuration at [frontend/tsconfig.json][] has a few customizations, many -to work with next.js. Relay has enabled the strict mode, which turns on a -default set of rules. The types are checked during the frontend build and -with `next lint`. +The configuration at [frontend/tsconfig.json][] has a few customizations. +Many of the changes help TypeScript work with `next.js`. Relay has enabled +strict mode, which turns on a default set of rules. The types are checked +during the frontend build and with `next lint`. [TypeScript]: https://www.typescriptlang.org/ [frontend/tsconfig.json]: https://github.com/mozilla/fx-private-relay/blame/main/frontend/tsconfig.json @@ -113,15 +112,14 @@ with `next lint`. Next.js [integrates with ESLint][nextjs-eslint]. It identifies mistakes and standardizes constructs in JavaScript. It can fix some issues automatically. -Relay has used ESLint since at least October 2020, and continued using it with +Relay added ESLint in October 2020 or earlier. Relay continued using it with the next.js refactor in March 2022. Relay uses Next.js's base ESLint configuration. This includes the tuning needed to make [ESLint][] formatting compatible with `prettier`. A stricter rule set that checks against [Google's Web Vitals][web-vitals] is also available. Relay -has expanded [frontend/.eslintrc.js][] to support our non-standard -configuration (static HTML generation with a Django server rather than a -next.js server). +uses Django rather than `next.js`. Relay expanded [frontend/.eslintrc.js][] to +support this non-standard configuration. [ESLint]: https://eslint.org/docs/latest/ [nextjs-eslint]: https://nextjs.org/docs/pages/building-your-application/configuring/eslint @@ -131,51 +129,46 @@ next.js server). ### black [black][] is a formatting tool to enforce layout of Python code. It rewrites -files in place, and reports if any files were reformatted. Relay adopted it in -May 2022. +files in place. Relay adopted it in May 2022. `black` uses [pyproject.toml][] for configuration. Relay uses the default -configuration, but may add the minimum expected Python version to allow -using more recent syntax. +configuration. In the future, Relay can tune the supported Python versions. [black]: https://black.readthedocs.io/en/stable/index.html [pyproject.toml]: https://github.com/mozilla/fx-private-relay/blob/main/pyproject.toml ### mypy -[mypy][] is a static type checker for Python. Relay added mypy support in April -2022, using an optional checking configuration as appropriate for an -[existing codebase][mypy-legacy]. +[mypy][] is a static type checker for Python. Relay added mypy support in April 2022. Relay started with an configuration for an +[existing codebase][mypy-existing]. `mypy` uses [pyproject.toml][] for configuration. Relay ignores issues with third party libraries that do not ship type hints. Relay disables strict rules -that require code changes. These changes are made incrementally as technical +that need code changes to pass. Relay ratchets up mypy strictness as technical debt projects. [mypy]: https://mypy.readthedocs.io/en/stable/ -[mypy-legacy]: https://mypy.readthedocs.io/en/stable/existing_code.html +[mypy-existing]: https://mypy.readthedocs.io/en/stable/existing_code.html ## Adding A New Linter Adding a new linter is not free. Each developer needs to add it to their workflow. -Continuous Integration needs to run the linter on each pull request. Linting -issues must be addressed before merging. +Continuous Integration needs to run the linter on each pull request. Developers +must address linting issues before merging. The decision to add a linter has two parts. First, is a linter needed? Second, -which linter should be added? +which linter? ### Decision Drivers: Is a Linter Needed? When deciding _if a linter is needed_, some criteria are: -- **Is the language used in production?** It is useful to find issues in this - code before it is shipped to users. +- **Is the language used in production?** Issues in production code can impact + users. A few seconds per pull request is worth avoiding a production bug. - **What is the impact of issues detected by the linter?** The highest impact - issues are user-facing bugs, and some tools address this directly. Often a - linter's purpose is to make code more uniform, which has a measurable - impact on the speed and effectiveness of code reviews. Reviewers can spend - time understanding the logic of code changes without the distraction of - parsing. + is avoiding a user-facing bug. The next level of impact is improving the + speed and effectiveness of code reviews. When tools handle the mechanical + review, reviewers spend their time on the logic of the code changes. Here is the linter chart again, focusing on areas without linters: @@ -187,29 +180,29 @@ Here is the linter chart again, focusing on areas without linters: | Standardize Constructs | yes | yes | **no** | no | no | | Spelling and Grammar | no | no | no | no | no | -Relay does not need more linters around Markdown at this time. Our -documentation is developer-facing, not user-facing. Errors in these documents -can be fixed on first review, and as they are encountered. There are tools like -the [markdownlint demo][] that can be used as needed. +Relay does not suffer from a lack of Markdown linters. Our documentation is +developer-facing, not user-facing. Reviewers find the most obvious errors. A +document with a few typos is still useful and easy to fix. Authors and +reviewers can use tools like the [markdownlint demo][] when needed. -Relay does not need shell script linters at this time. These scripts are used -in CircleCI to automate build and test steps. Errors in these scripts appear as -broken builds, and are fixed as part of normal development. They do not have a -direct impact on our users. There are tools like [ShellCheck][] that can be -used as needed. +Relay does not suffer from a lack of shell script linters. Relay uses shell +scripts in CircleCI for build and test steps. Errors in these scripts appear as +broken builds. Developers fix them to unblock the build. These build failures +do not have a direct impact on our users. Authors and reviewers can use tools +like [ShellCheck][] when needed. -Relay does not need linters around spelling and grammar at this time. -User-facing strings are translated, and are reviewed closely. A good code -comment with a spelling error can still be understood by another developer. A -perfectly formed but misleading comment should be deleted. There are tools -like [PyEnchant][], [Hemmingway][], and word processors that can be used by -developers without being checked in continuous integration. +Relay does not suffer from spelling and grammar errors. Many reviewers check +user-facing strings, as they proceed from design to translation. Errors in +code comments and function names do not cause bugs. A developer understands a +good code comment with a spelling error. A developer should delete a misleading +comment with perfect English. Authors and reviewers can use tools like +[PyEnchant][], [Hemingway][], and word processors when needed. As highlighted, there is a gap in Python linters. There is no official linter to identify mistakes and standardize constructs. Python is used in production for the web and API server, as well as background tasks. Errors in this code will impact users, and non-standard code can slow down code reviews. This gap -meets the criteria for adding a new linter to CI and development. +meets the criteria for adding a new linter to CircleCI and development. [markdownlint demo]: https://dlaa.me/markdownlint/ [ShellCheck]: https://www.shellcheck.net/ @@ -218,89 +211,83 @@ meets the criteria for adding a new linter to CI and development. ## Decision Drivers for Choosing Between Similar Linters -A linter that is required for Relay development must have these attributes: +A required linter must have these attributes: -- **Checks on pull request** - The tool needs to run in the continuous - integration (CI) environments, so that no failing code is merged to main. -- **Runs in the development environment** - Developers need to be able to - run the tool locally. At a minimum, a developer should be able to run the - tool once per commit. The developer should be able to run the tool manually - as well. A developer should have high confidence that the tool runs the - same locally as in CI. -- **Marks false positives** - There should be few false positives, where the +- **Checks on pull request**: The continuous integration process needs to run + the tool. If the tool identifies an issue, the build should fail. This + prevents merging failing code. +- **Runs in the development environment**: Fast tools should run as a pre-commit step. + A developer can run the tool on their machine. When the tool accepts the code, it should also pass in CI. +- **Marks false positives**: There should be few times where the tool identifies a problem but the code is OK. When there is a false positive, it should be possible to ignore it and get a passing check. When choosing between similar tools, these attributes can help guide the decision: -- **Good defaults** - If a tool has good default settings, it is more likely - that it will need less project-specific configuration. Developers can use - tips and tricks from other projects using the tool. -- **Fixes issues when appropriate** - A tool that enforces layout is more - useful when it can rewrite the source code to the correct layout. The - alternative, only identifying layout issues, makes for a worse developer - experience. In other cases, it is better to identify the issue and let the - developer decide how to fix it. -- **Editor integration** - When a tool is incorporated in the developer's code - editor, enforcing layout and fixing issues becomes a natural part of the - code writing process. -- **Speed** - A faster tool will be run earlier in the development process. If - a tool runs in a millisecond, it can be incorporated into the editor, - similar to syntax highlighting. If a tool runs in 100 milliseconds, it can - be run every time the file is saved. If a tool takes a minute to run, - developers will avoid running it locally, and wait for CI to identify - issues. +- **Good defaults**: A tool with good defaults needs less configuration. + Developers can use tips and tricks from other projects using the tool. +- **Fixes issues when appropriate**: Developers love a "fix it" button. + A code formatter is better than a tool that identifies formatting problems. A + tool that can fix problems correctly is better than a tool that only + identifies them. A developer should fix issues that need human judgement. +- **Editor integration**: A developer's primary tool is the code editor. A + integrated tool helps fix issues as part of the writing process. A + non-integrated tool turns code linting into an extra chore. +- **Speed**: A fast tool gets used. A slow tool gets skipped. A tool than runs + on each file save should take less than a second. A pre-commit hook should + run in less than five seconds. Developers should feel they save time running + a tool, rather than waiting for CI. There are many tools that address identifying mistakes and standardizing constructs in Python. Some linters that would fill this role: -- [pylint][] - This tool has been in development since 2003, and has +- [pylint][]: This tool has been in development since 2003. It has hundreds of built-in checks, including layout enforcement and spelling checkers. It works across entire codebases instead of individual files. It identifies 2,800 issues in our code, in 25 seconds. -- [pycodestyle][] - This tool, formerly known as `pep8`, has been in - development since 2006, and is focused on code formatting and some code +- [pycodestyle][]: This tool, formerly known as `pep8`, has been in + development since 2006. It is focused on code formatting and some code constructs. It identifies 317 issues in our code in 1.5 seconds. -- [pyflakes][] - This tool has been in development since 2009, targeted - at identifying mistakes in code without caring about formatting. It finds - 12 issues in our code in 12 seconds. -- [flake8][] - This tool has been in development since 2010, and combines +- [pyflakes][]: This tool has been in development since 2009. It identifies + mistakes in code without caring about formatting. It finds 12 issues in our + code in 12 seconds. +- [flake8][]: This tool has been in development since 2010. It combines `pycodestyle`, `pyflakes` and the `mccabe` tool into one package. It also has a plugin system that can extend the ruleset. It identifies 277 issues in our code in 600 milliseconds. -- [isort][] - This tool has been in development since 2017, and reformats - imports to enforce a sort order and import style. It detects 99 issues in - our code in 750 milliseconds. It can be used as a `flake8` plugin. -- [bandit][] - This tool scans code for security issues. It finds 1997 low +- [isort][]: This tool has been in development since 2017. It reformats + imports to enforce a order and style. It detects 99 issues in our code in + 750 milliseconds. It can be used as a `flake8` plugin. +- [bandit][]: This tool scans code for security issues. It finds 1997 low severity and 8 medium severity issues in our code in 1.5 seconds. It can be run as a `flake8` plugin. -- [ruff][] - This tool, launched in 2023 and written in - Rust, is optimized for speed and automatic fixing. With the default rules, - which implement some `flake8` rules, it identifies 26 issues in our code, 9 - fixable, in 35 milliseconds. It has optional rules that implement the - checks of all the previous tools. - -All the tools have the required attributes. They can be run in CI and in -the development environment. They are configurable, and have a mechanism for +- [ruff][]: This tool, launched in 2023 and written in Rust, is designed for + speed and automatic fixing. It identifies 26 issues in our code, 9 fixable, + in 35 milliseconds. It has optional rules that implement the checks of all + the previous tools. + +All the tools have the required attributes. They run in CI and in the +development environment. They are configurable, and have a mechanism for marking false positives. Three approaches that stand out: -- `pylint` for identifying the most issues. The defaults will need to be - tuned to remove false positives, such as warnings on `assert` in tests. It - is slow enough to cause pain if used for local development. +- `pylint` for identifying the most issues. There are many false positives, such + as missing documentation and understanding pytest fixtures. Significant + configuration will cut these false positives. It is slow enough to + cause pain if used for local development. - `flake8` with several plugins (`isort`, `bandit`, others). This is a - good mix of speed and coverage. The tools and plugins are maintained by - [PyCQA][] and integrate well together. + good mix of speed and coverage. [PyCQA][] maintains the tools and plugins, + and they work well together. - `ruff` with additional checks enabled. This tool can perform many of the checks that the other tools check. It runs 10x - 100x faster than the other tools. It also is compatible with `black` formatting by default. - The largest negative is that [Astral][], the company that develops - `ruff`, is VC-backed, and the tool may add monetization in the future. + The largest negative is that [Astral][] is VC-backed. The tool may add + monetization in the future. Due to the speed and flexibility, `ruff` is the recommended tool for the -next linter, covering the important linting gap for Python. +next linter. [Astral]: https://astral.sh/blog/announcing-astral-the-company-behind-ruff [PyCQA]: https://github.com/PyCQA From f11912cd7682e284ef3de6c4761ef0f9b7794917 Mon Sep 17 00:00:00 2001 From: John Whitlock Date: Thu, 4 Apr 2024 16:25:44 -0500 Subject: [PATCH 4/6] fourth draft --- docs/adr/0004-use-code-linters.md | 102 +++++++++++++++--------------- 1 file changed, 50 insertions(+), 52 deletions(-) diff --git a/docs/adr/0004-use-code-linters.md b/docs/adr/0004-use-code-linters.md index 4669b5ad5f..a5a66b4d93 100644 --- a/docs/adr/0004-use-code-linters.md +++ b/docs/adr/0004-use-code-linters.md @@ -15,34 +15,32 @@ addressed by different linting tools include: - **Enforce Layout**: A tool can check and even reformat code to fit a project style. This helps make code and code changes easier to read and comprehend. - It also allows new contributors to match the existing project style. + It helps new contributors to match the existing project style. - **Check Types**: Dynamic languages are flexible about variable types. Valid code with incorrect types can result in bugs and runtime errors. A linter can check type hints for consistent usage. Code editors can use type hints for documentation and assisted code writing. -- **Identify Mistakes**: A tool can detect some common mistakes. Examples are +- **Identify Mistakes**: A tool can detect common mistakes. Examples are identifying unused variables, and using loose equality in JavaScript. - **Standardize Constructs**: There are many ways to express the same logic in code. A linter can suggest or rewrite code to a standard form. One example is the order and placement of imports. Another is omitting optional defaults to a function call. -- **Spelling and Grammar Checking**: A tool can check for misspelling and grammar - mistakes. The tools have different priorities from similar tools for word - processors. The text can appear in prose and code comments. The text includes - technical and project-specific terms. +- **Spelling and Grammar Checking**: These tools have different priorities from + similar tools for word processors. They expect technical and project-specific + terms. They look for text in prose and code comments. Most developers appreciate that linters make multi-developer projects easier. -Tool check and enforce project standards. Code reviewers focus on the logic of -changes, rather than the form of the code. A linter may identify an unexpected -issue. A developer can read the linter documentation to learn more about the -issue. This is one way a developer can improve their understanding of the +Tools automatically check and enforce project standards. Code reviewers focus +on the logic of changes, rather than the form of the code. A linter can teach +developers about unexpected issues and improve their understanding of the project's languages. Also, developers dislike changes to their working process. The benefits may be -unclear. They may disagree that the cost is worth the benefits. It becomes -harder to add a tool as the lines of code and the number of team members grow. -The best time to incorporate linters is at the start of the project. Relay did -not, so adding these tools has been a slower, more deliberate process. +unclear. They may disagree that the benefits are worth the extra effort. These +objections are easiest to overcome at the start of a project. As a project +becomes bigger, it becomes harder to satisfy a new tool. Adding a new tool to +a mature project is a slow, deliberate process. ## Linters Used By Relay @@ -126,6 +124,18 @@ support this non-standard configuration. [web-vitals]: https://web.dev/articles/vitals [frontend/.eslintrc.js]: https://github.com/mozilla/fx-private-relay/blob/main/frontend/.eslintrc.js +### mypy + +[mypy][] is a static type checker for Python. Relay added mypy support in April 2022. Relay started with a recommended configuration for an +[existing codebase][mypy-existing]. + +`mypy` uses [pyproject.toml][] for configuration. Relay ignores issues with +third party libraries that do not ship type hints. We disable strict rules +that need code changes to pass. We ratchet up `mypy` strictness over time. + +[mypy]: https://mypy.readthedocs.io/en/stable/ +[mypy-existing]: https://mypy.readthedocs.io/en/stable/existing_code.html + ### black [black][] is a formatting tool to enforce layout of Python code. It rewrites @@ -137,24 +147,11 @@ configuration. In the future, Relay can tune the supported Python versions. [black]: https://black.readthedocs.io/en/stable/index.html [pyproject.toml]: https://github.com/mozilla/fx-private-relay/blob/main/pyproject.toml -### mypy - -[mypy][] is a static type checker for Python. Relay added mypy support in April 2022. Relay started with an configuration for an -[existing codebase][mypy-existing]. - -`mypy` uses [pyproject.toml][] for configuration. Relay ignores issues with -third party libraries that do not ship type hints. Relay disables strict rules -that need code changes to pass. Relay ratchets up mypy strictness as technical -debt projects. - -[mypy]: https://mypy.readthedocs.io/en/stable/ -[mypy-existing]: https://mypy.readthedocs.io/en/stable/existing_code.html - ## Adding A New Linter Adding a new linter is not free. Each developer needs to add it to their workflow. -Continuous Integration needs to run the linter on each pull request. Developers -must address linting issues before merging. +Continuous Integration (CI) needs to run the linter on each pull request. +Developers must address linting issues before merging. The decision to add a linter has two parts. First, is a linter needed? Second, which linter? @@ -167,8 +164,9 @@ When deciding _if a linter is needed_, some criteria are: users. A few seconds per pull request is worth avoiding a production bug. - **What is the impact of issues detected by the linter?** The highest impact is avoiding a user-facing bug. The next level of impact is improving the - speed and effectiveness of code reviews. When tools handle the mechanical - review, reviewers spend their time on the logic of the code changes. + speed and effectiveness of code reviews. Tools can handle the mechanical + review. Reviewers are free to spend their time and attention on the logic of + the code changes. Here is the linter chart again, focusing on areas without linters: @@ -192,7 +190,7 @@ do not have a direct impact on our users. Authors and reviewers can use tools like [ShellCheck][] when needed. Relay does not suffer from spelling and grammar errors. Many reviewers check -user-facing strings, as they proceed from design to translation. Errors in +new user-facing strings, as they proceed from design to translation. Errors in code comments and function names do not cause bugs. A developer understands a good code comment with a spelling error. A developer should delete a misleading comment with perfect English. Authors and reviewers can use tools like @@ -201,43 +199,43 @@ comment with perfect English. Authors and reviewers can use tools like As highlighted, there is a gap in Python linters. There is no official linter to identify mistakes and standardize constructs. Python is used in production for the web and API server, as well as background tasks. Errors in this code -will impact users, and non-standard code can slow down code reviews. This gap -meets the criteria for adding a new linter to CircleCI and development. +will impact users, and non-standard code can slow down code reviews. Relay +should add a new tool to address this gap. [markdownlint demo]: https://dlaa.me/markdownlint/ [ShellCheck]: https://www.shellcheck.net/ [PyEnchant]: https://pyenchant.github.io/pyenchant/# [Hemingway]: https://hemingwayapp.com/ -## Decision Drivers for Choosing Between Similar Linters +## Decision Drivers: Which Linter? A required linter must have these attributes: - **Checks on pull request**: The continuous integration process needs to run the tool. If the tool identifies an issue, the build should fail. This prevents merging failing code. -- **Runs in the development environment**: Fast tools should run as a pre-commit step. - A developer can run the tool on their machine. When the tool accepts the code, it should also pass in CI. -- **Marks false positives**: There should be few times where the - tool identifies a problem but the code is OK. When there is a false - positive, it should be possible to ignore it and get a passing check. +- **Runs in the development environment**: Fast tools should run as a pre-commit + step. A developer can run the tool on their machine. When the tool accepts the + code, it should also pass in CI. +- **Marks false positives**: It should be rare to identify good code as causing + a problem. When there is a false positive, it should be possible to ignore it + and get a passing check. When choosing between similar tools, these attributes can help guide the decision: - **Good defaults**: A tool with good defaults needs less configuration. - Developers can use tips and tricks from other projects using the tool. -- **Fixes issues when appropriate**: Developers love a "fix it" button. - A code formatter is better than a tool that identifies formatting problems. A + The tool works the same across projects. +- **Fixes issues when appropriate**: Developers love a "fix it" button. A tool that can fix problems correctly is better than a tool that only - identifies them. A developer should fix issues that need human judgement. -- **Editor integration**: A developer's primary tool is the code editor. A + identifies them. A developer should fix issues that need human judgment. +- **Editor integration**: A developer's primary tool is the code editor. An integrated tool helps fix issues as part of the writing process. A non-integrated tool turns code linting into an extra chore. -- **Speed**: A fast tool gets used. A slow tool gets skipped. A tool than runs +- **Speed**: A fast tool gets used. A slow tool gets skipped. A tool that runs on each file save should take less than a second. A pre-commit hook should run in less than five seconds. Developers should feel they save time running - a tool, rather than waiting for CI. + a tool, rather than waiting to see if it fails in CI. There are many tools that address identifying mistakes and standardizing constructs in Python. Some linters that would fill this role: @@ -257,7 +255,7 @@ constructs in Python. Some linters that would fill this role: has a plugin system that can extend the ruleset. It identifies 277 issues in our code in 600 milliseconds. - [isort][]: This tool has been in development since 2017. It reformats - imports to enforce a order and style. It detects 99 issues in our code in + imports to enforce an order and style. It detects 99 issues in our code in 750 milliseconds. It can be used as a `flake8` plugin. - [bandit][]: This tool scans code for security issues. It finds 1997 low severity and 8 medium severity issues in our code in 1.5 seconds. It can be @@ -277,12 +275,12 @@ Three approaches that stand out: as missing documentation and understanding pytest fixtures. Significant configuration will cut these false positives. It is slow enough to cause pain if used for local development. -- `flake8` with several plugins (`isort`, `bandit`, others). This is a +- `flake8` with several plugins (`isort`, `bandit`, others). This provides a good mix of speed and coverage. [PyCQA][] maintains the tools and plugins, and they work well together. -- `ruff` with additional checks enabled. This tool can perform many of the - checks that the other tools check. It runs 10x - 100x faster than the - other tools. It also is compatible with `black` formatting by default. +- `ruff` with additional checks enabled. This tool can enforce many of the + rules of the other tools in a single package. It runs 10x - 100x faster than + the other tools. It is compatible with `black` formatting by default. The largest negative is that [Astral][] is VC-backed. The tool may add monetization in the future. From cfbeff01f59aa5d064a12281f851a8fa580759c5 Mon Sep 17 00:00:00 2001 From: John Whitlock Date: Thu, 4 Apr 2024 16:27:20 -0500 Subject: [PATCH 5/6] set status --- docs/adr/0004-use-code-linters.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/adr/0004-use-code-linters.md b/docs/adr/0004-use-code-linters.md index a5a66b4d93..d608e88a23 100644 --- a/docs/adr/0004-use-code-linters.md +++ b/docs/adr/0004-use-code-linters.md @@ -1,6 +1,6 @@ # Use Code Linters to Enforce Layout and Discover Issues -- Status: Informational +- Status: Proposed - Deciders: Luke Crouch - Date: 2024-04-04 From 748a2a9a7d6be172308ba9bd78447f0a1ea3bcf2 Mon Sep 17 00:00:00 2001 From: John Whitlock Date: Wed, 10 Apr 2024 17:13:23 -0500 Subject: [PATCH 6/6] Update status to accepted --- docs/adr/0004-use-code-linters.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/adr/0004-use-code-linters.md b/docs/adr/0004-use-code-linters.md index d608e88a23..a8d15996f5 100644 --- a/docs/adr/0004-use-code-linters.md +++ b/docs/adr/0004-use-code-linters.md @@ -1,8 +1,8 @@ # Use Code Linters to Enforce Layout and Discover Issues -- Status: Proposed -- Deciders: Luke Crouch -- Date: 2024-04-04 +- Status: Accepted +- Deciders: Luke Crouch, Se Yeon Kim, John Whitlock +- Date: 2024-04-10 Technical Story: [MPP-79][]