Skip to content

Commit

Permalink
Second pass
Browse files Browse the repository at this point in the history
  • Loading branch information
@gdjohnson
gdjohnson committed Apr 25, 2024
1 parent 7f5e00f commit de52dc1
Showing 1 changed file with 83 additions and 69 deletions.
152 changes: 83 additions & 69 deletions knowledge_base/Linting.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,39 @@

## Contents

- [Commands](#commands)
- [Configuration](#configuration)
- [Formatting](#formatting)
- [Formatting Commands](#formatting-commands)
- [ESLint](#eslint)
* [Commands](#commands)
* [Configuration](#configuration)
- [Prettier Formatting](#prettier-formatting)
* [Formatting Commands](#formatting-commands)
- [Handling Linting Issues: Cheatsheet](#handling-linting-issues-cheatsheet)
* [Incorrect Use Of Promises](#incorrect-use-of-promises)
+ [Not Awaiting Promises](#not-awaiting-promises)
+ [Passing Async Functions To Event Handlers](#passing-async-functions-to-event-handlers)
* [No Explicit Any](#no-explicit-any)
+ [REST API Calls That Aren't Typed](#rest-api-calls-that-arent-typed)
* [Disabling Rules](#disabling-rules)
* [Disabling Next Line](#disabling-next-line)
+ [Turning Off Eslint Diff](#turning-off-eslint-diff)
* [Things Not To Do](#things-not-to-do)
+ [Do Not Disable Lint On The Entire File](#do-not-disable-lint-on-the-entire-file)
+ [Do Not Disable The Entire Next Line](#do-not-disable-the-entire-next-line)
- [Change Log](#change-log)

## ESLint

We use ESLint and the eslint-diff plugin for linting.

### Commands

All linting commands should be run from root.

- `yarn lint-all`
- Lints all code (regardless of changes)
- `yarn lint-branch`
- Lints all changes made in a branch (used in CI).
- Lints all changes made in a branch (used in CI)

## Configuration
### Configuration

There is a single root `.eslintrc.js` file. All packages inherit from this config. Child configs in specific packages can modify/override the rules as well as the settings defined by the parent config.

Expand All @@ -28,161 +43,160 @@ Global rules we eventually want to turn back on:
- `@typescript-eslint/no-explicit-any`
- `@typescript-eslint/consistent-type-imports`

We will slowly remove rules turned off in the Commonwealth `.eslintrc.js`

### Plugins
We will slowly remove rules turned off in the Commonwealth `.eslintrc.js`.

We use an `eslint-diff` plugin that only applies rules to changed files. This runs in our CI.
Configuration for the eslint-diff plugin is housed in [.eslintrc-diff.js](../packages/commonwealth/.eslintrc-diff.js).

## Prettier Formatting

We use Prettier for formatting our codebase. There is only 1 prettier config and it is `.prettierrc.json` at the root of the repo.
We use Prettier for formatting our codebase. There is only 1 Prettier config and it is `.prettierrc.json` at the root of the repo.

*Never run prettier from anywhere other than root to avoid conflicting prettier configs!*
*Never run Prettier from anywhere other than root to avoid conflicting Prettier configs!*

Prettier is not enforced by our CI, but runs during precommit via a Husky hook, which formats all files diverging from master. `yarn install` must have been run on your machine for the precommit hook to work.

### Formatting Commands

Formatting commands should **always** be executed from root.
Formatting commands should *always* be executed from root.

- `yarn format`
- execute prettier and format the entire repo
- Execute Prettier and format the entire repo
- `yarn format-check`
- execute prettier in check mode which means it lists files that require formatting but doesn't actually format them

## Handling linting issues

### Background
- Execute Prettier in `check` mode, so that it lists all files requiring formatting, but doesn't automatically format them

We run `eslint` and the `eslint-diff` plugin to find issues with our source code which augment the typescript compiler.
## Handling Linting Issues: Cheatsheet

Lint issues have traditionally/initially involved minor issues with spacing but now they can catch very real and very serious bugs.
*Written by Kevin Burton, 240422.*

As of 240423, common bugs in our codebase include React and promise bugs that we’re trying to block.
As of 240422, common bugs in our codebase include React and promise bugs that we’re trying to block. Lint issues have traditionally/initially involved minor issues with spacing but now they can catch very real and very serious bugs.

## Incorrect Use of Promises
### Incorrect Use Of Promises

This is a common one and these are almost certainly bugs. They are mostly just due to dangling promises where you’re either not calling `await` or `catch`.
This is a common issue, and these are almost certainly bugs. They are mostly due to dangling promises where you’re either not calling `await` or `catch`.

The solution is to either call catch() then log the error or you have to call `await` and have it in an `async` function.
The solution is to either call `catch()`, then log the error, or to call `await` and wrap it in an `async` function.

Here’s are a few examples
Here are a few examples:

**Not awaiting promises:**
#### Not Awaiting Promises

```
```ts
async function doSubmit() {
sendDataToServer(data)
sendDataToServer(data);
}
```

This function is an async function that calls sendDataToServer but there is no await there.
This function is an async function that calls `sendDataToServer` but lacks an `await`.

Instead you should change it to:
It should be changed to:

```
```ts
async function doSubmit() {
await sendDataToServer(data)
await sendDataToServer(data);
}
```

**Passing async functions to event handlers**
#### Passing Async Functions To Event Handlers

This is another common problem

```
```tsx
async function sendDataToServer() {
// send the data to the server using the fetch API
// send the data to the server using the fetch API
}

return (
<button onClick={sendDataToServer}>Submit</button>
);
```

There’s no catch() called on sendDataToServer so if an error happens the user is never notified.
There’s no `catch()` called on `sendDataToServer`, so if an error occurs, the user is never notified.

You can fix it by doing the following:
It can be fixed with the following:

```
```tsx
function handleClick() {
sendDataToServer()
.catch(err => log.error(err)}
.catch(err => log.error(err));
}

return (
<button onClick={handleClick>Submit</button>
<button onClick={handleClick}>Submit</button>
);
```

## No Explicit Any
### No Explicit Any

This will happen when you use the ‘any’ in code for variables and types.
This will happen when you use `any` in code for variables and types.

Honestly the main solution is to NEVER use `any`. 95% of the time you or someone else is going to shoot themselves in the foot. It’s usually a future developer though or usually an issue with
The main solution is to *never* use `any`. 95% of the time, you or someone else is going to shoot themselves (or a future developer) in the foot.

There are some reasonable exceptions though:

**REST API calls that aren’t typed**
#### REST API Calls That Aren't Typed

In this situation a workable solution is to define types inline, then use those types, then disable the next line in eslint with just the specific rule violation.

# Disabling Rules
### Disabling Rules

If you can’t properly fix an issue you MUST disable the next line using the official eslint mechanism to disable the next line.
If you can’t properly fix an issue you *must* disable the next line using the official eslint mechanism to disable the next line.

There is an explicit mechanism to do this in eslint and it’s really easy to use and just requires you to add a comment.

You DO NOT need to do anything fancy to disable the rule.
You *do not* need to do anything fancy to disable the rule.

## Disable next line
### Disabling Next Line

The main way to fix an eslint issue is to just disable the next line with a specific rule.

For example:

```
```ts
// eslint-disable-next-line @typescript-eslint/no-misused-promises
```

Note the fact that this JUST disables the `@typescript-eslint/no-misused-promises`for promises handling

<p id="gdcalert1" ><span style="color: red; font-weight: bold">>>>>> gd2md-html alert: inline image link here (to images/image1.png). Store image on your image server and adjust path/filename/extension if necessary. </span><br>(<a href="#">Back to top</a>)(<a href="#gdcalert2">Next alert</a>)<br><span style="color: red; font-weight: bold">>>>>> </span></p>

![alt_text](images/image1.png "image_tooltip")
```tsx
<CWText type="b1" fontWeight="medium">
{formatAddressShort(contractAddress, 5, 5)}
<CWIcon
className="copy-icon"
iconName="copyNew"
// eslint-disable-next-line @typescript-eslint/no-misused-promises
onClick={async () => await saveToClipboard(contractAddress, true)}
/>
</CWText>
```

You **must** include the rule in to disable just that one specific rule.
You *must* include the rule in to disable just that one specific rule.

Ideally you would also explain why you’re disabling the rule as well.

There are valid reasons why you would want to
#### Turning Off Eslint Diff

## Turning Off Eslint Diff

If something goes wrong and you need to totally disable eslint-diff you can just change it in CI.yml
If something goes wrong and you need to totally disable eslint-diff you can just change it in `CI.yml`.

You can just look for the following code

```yml
# To disable eslint-diff just comment the following two lines
- name: Run eslint-diff
run: yarn workspace commonwealth run lint-diff
```
# To disable eslint-diff just comment the following two lines
- name: Run eslint-diff
run: yarn workspace commonwealth run lint-diff
```

# Things NOT to do
## Do not disable lint on the entire file
### Things Not To Do
Do not add a // estlint-disable comment in the entire file. This will disable lint-diff moving forward and any issues.
#### Do Not Disable Lint On The Entire File
ALWAYS use eslint-disable-next line so we lower the blast radius.
Do not add a `// estlint-disable` comment to the entire file. This will disable `lint-diff` moving forward, and any issues it might raise. *Always* use `eslint-disable-next-line` so we lower the blast radius.

## Do NOT disable the entire next line (regardless of error code)
#### Do Not Disable The Entire Next Line

If you JUST use //eslint-disable-next-line and new bugs are introduced in the future, eslint won’t generate an error on those lines.
Regardless of error code, the entire next line should never be disabled with `//eslint-disable-next-line`. If new bugs are introduced in the future, eslint won’t generate an error on those lines.

## Change Log

- 240425: Linting cheatsheet added by Kevin Burton and Graham Johnson.
- 231012: Flagged by Graham Johnson for review. Recommendations out of date; greater clarity and context desirable.
- 230830: Split off by Graham Johnson from [Code-Style](./Code-Style.md) entry.

0 comments on commit de52dc1

Please sign in to comment.