Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Help new starters fall into the pit of success #115

Open
Cruikshanks opened this issue Apr 30, 2024 · 0 comments
Open

Help new starters fall into the pit of success #115

Cruikshanks opened this issue Apr 30, 2024 · 0 comments
Assignees
Labels
enhancement New feature or request

Comments

@Cruikshanks
Copy link
Member

Cruikshanks commented Apr 30, 2024

The idea that a well-designed system makes it easy to do the right things and annoying to do the wrong things. Falling into "the pit of success" means setting up your system/practices so that it's easy to do the right, desired, successful things.

Hackterms
Pit of success coined by Rico Mariani

We've been fortunate enough to grow as a team this year which means we've had several new developers join us! 🥳

Unfortunately, it has exposed our onboarding still leaves a lot to be desired. We find we keep having to highlight the same coding convention or stylistic issues in new members' initial PRs. This is irrespective of whether we have written up the convention or not.

Wouldn't it be great if we used all this computing power we interact with daily to help them get it right the first time? That we help them fall into the "pit of success"?

So, this is a catch-all issue for ideas and changes that will help make their and our lives easier and help all succeed first-time.

Be wary though! It can be easy to go too far and start forcing people to work in ways they do not wish. For example, forcing them to use a specific code editor, or insisting on automating processes that can fail and cause folks to be blocked.

We've already decided to move from StandardJS to ESLint and the StandardJS ruleset. There are things StandardJS doesn't highlight that we can with ESLint.

Another idea would be to add an .editorconfig to the repo. Folks have the choice to add the extension (some IDEs will use it out of the box) but it at least defines some basics for any code files folks wish to add.

We only intend to apply these ideas to water-abstraction-system. The legacy code is not worth the time and effort.


Things we've adopted

  • switch to using ESLint so we can add additional rules
  • adding a .editorconfig to the repo

Things we've rejected

  • git commit hooks to run linting, tests etc using something like Husky
    • Our CI will ensure linting and unit tests are run which will prevent non-conventional code from being merged. If and when devs choose to run these tests locally should by down to them rather than being forced to run on every commit
@Cruikshanks Cruikshanks self-assigned this Apr 30, 2024
@Cruikshanks Cruikshanks added housekeeping Refactoring, tidying up or other work which supports the project enhancement New feature or request and removed housekeeping Refactoring, tidying up or other work which supports the project labels Apr 30, 2024
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue Apr 30, 2024
DEFRA/water-abstraction-team#115

We've been fortunate enough to grow as a team this year which means we've had several new developers join us! 🥳

Unfortunately, it has exposed our onboarding still leaves a lot to be desired. We find we keep having to highlight the same coding convention or stylistic issues in new members' initial PRs. This is irrespective of whether we have written up the convention or not.

Wouldn't it be great if we used all this computing power we interact with daily to help them get it right the first time? That we help them fall into the [pit of success](https://www.hackterms.com/pit%20of%20success)?

One idea is to add an [.editorconfig](https://editorconfig.org/) to the repo. Folks have the choice to add the extension (some IDEs will use it out of the box) but it at least defines some basics for any code files folks wish to add.

This change adds an initial version generated using the [VSCode extension](https://marketplace.visualstudio.com/items?itemName=EditorConfig.EditorConfig) based on current editor settings.
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue May 1, 2024
DEFRA/water-abstraction-team#115

We've been fortunate enough to grow as a team this year which means we've had several new developers join us! 🥳

Unfortunately, it has exposed our onboarding still leaves a lot to be desired. We find we keep having to highlight the same coding convention or stylistic issues in new members' initial PRs. This is irrespective of whether we have written up the convention or not.

Wouldn't it be great if we used all this computing power we interact with daily to help them get it right the first time? That we help them fall into the [pit of success](https://www.hackterms.com/pit%20of%20success)?

One idea is to add an [.editorconfig](https://editorconfig.org/) to the repo. Depending on folks IDE choice it will either be recognised out-of-the-box or they'll need an extension. 

Unlike other tools having the file doesn't do anything _directly_ to your project. Instead, it tells your chosen editor how to behave when working with the files. For example, when you indent something whether to use tabs or spaces and how many. If your editor can remove whitespace on save, the `.editorconfig` will tell it whether to do that or not.

Even for folks using an incompatible editor or who choose not to install the extension, we at least have something that defines what we expect and can be used in our CI pipeline.

This change adds an initial version generated using the [VSCode extension](https://marketplace.visualstudio.com/items?itemName=EditorConfig.EditorConfig) based on current editor settings.

It also updates our GitHub CI workflow to check that pushed changes match our conventions.

---

This change highlighted that the line endings in our migrations were inconsistent. So, the fix for those is included.
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue May 8, 2024
DEFRA/water-abstraction-team#115

In [Add linting using eslint with standard as the base](#948) we switched from using [standard](https://www.npmjs.com/package/standard) directly for linting to using [ESlint](https://eslint.org/).

This is all part of work we are doing to start codifying our conventions rather than writing them up for no one to read! 😁

As a start, we enabled the rule [max-len](https://eslint.org/docs/latest/rules/max-len) which **standard** doesn't have. That flagged some long text strings we have, and we don't want to break them up just for the sake of the linter.

So, we went to check what options there are for the rule and found this

> This rule was **deprecated** in ESLint v8.53.0. Please use the [corresponding rule](https://eslint.style/rules/js/max-len) in [@stylistic/eslint-plugin-js](https://eslint.style/packages/js).

But it looks like ESLint has moved on a bit since we last used it and has chosen to [move some of its core stylistic based rules to a plugin](https://eslint.style/packages/js).

This change switches to using the plugin to lint `max-len`. It also updates the config to exclude strings and template literals from the rule.
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue May 8, 2024
DEFRA/water-abstraction-team#115

In [Add linting using eslint with standard as the base](#948) we switched from using [standard](https://www.npmjs.com/package/standard) directly for linting to using [ESlint](https://eslint.org/).

This is all part of the work we are doing to start codifying our conventions rather than writing them up for no one to read! 😁

As a start, we enabled the rule [max-len](https://eslint.org/docs/latest/rules/max-len) which **standard** doesn't have. That flagged some long text strings we have, and we don't want to break them up just for the sake of the linter.

So, we went to check what options there were for the rule and found this

> This rule was **deprecated** in ESLint v8.53.0. Please use the [corresponding rule](https://eslint.style/rules/js/max-len) in [@stylistic/eslint-plugin-js](https://eslint.style/packages/js).

But it looks like ESLint has moved on a bit since we last used it and has chosen to [move some of its core stylistic-based rules to a plugin](https://eslint.org/blog/2023/10/deprecating-formatting-rules/).

This change switches to using the plugin to lint `max-len`.
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue May 8, 2024
DEFRA/water-abstraction-team#115

In [Add linting using eslint with standard as the base](#948) we switched from using [standard](https://www.npmjs.com/package/standard) directly for linting to using [ESlint](https://eslint.org/).

This is all part of the work we are doing to start codifying our conventions rather than writing them up for no one to read! 😁

We then went to make a config change but saw that we were using a deprecated core rule and instead needed to [use @stylistic/eslint-plugin-js for max-len rule](#989). So, we fixed that! 💪

But then we clocked ESLint was telling us our `.eslintrc.js` [config file format is deprecated](https://eslint.org/docs/latest/use/configure/configuration-files-deprecated)! 😩 We instead should be using the `eslint.config.js` [flat file config](https://eslint.org/docs/latest/use/configure/configuration-files).

So, before we start making changes to our config this change gets us using the latest supported format.
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue May 8, 2024
DEFRA/water-abstraction-team#115

Whilst we should be making every effort to keep lines shorter than 120 characters the intent for the rule is to stop folks from stringing together complex one-liners.

There are times we need to include long pieces of text, build strings from verbose variable names or reference URLS that will cause us to go over the limit.

We don't want to make them more complex just for the sake of a linter. So, we add this config to exclude these types from the rule.
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue May 9, 2024
DEFRA/water-abstraction-team#115

Whilst we should be making every effort to keep lines shorter than 120 characters the intent of the rule is to stop folks from stringing together complex one-liners.

Sometimes we need to include long pieces of text, build strings from verbose variable names or reference URLS that will cause us to exceed the limit.

We don't want to make them more complex just for the sake of a linter. So, we add this config to exclude these lines of these types from the rule.
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue May 12, 2024
DEFRA/water-abstraction-team#115

In [Exclude long strings from ESLint max-len rule](#992) we updated our ESLint [max-len](https://eslint.style/rules/js/max-len) to exclude long strings or template literals from being flagged as issues.

That reduced the noise (there were lots of infractions!) and helped us see actual long lines of logic which should be refactored.

This fixes all the remaining `max-len` infractions.
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue May 12, 2024
DEFRA/water-abstraction-team#115

In [Exclude long strings from ESLint max-len rule](#992) we updated our ESLint [max-len](https://eslint.style/rules/js/max-len) to exclude long strings or template literals from being flagged as issues.

That reduced the noise (there were lots of infractions!) and helped us see actual long lines of logic which should be refactored.

This fixes all the remaining `max-len` infractions.
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue May 12, 2024
DEFRA/water-abstraction-team#115

In [Exclude long strings from ESLint max-len rule](#992) we updated our ESLint [max-len](https://eslint.style/rules/js/max-len) to exclude long strings or template literals from being flagged as issues.

That reduced the noise (there were many infractions!) and helped us see long lines of logic that should be refactored.

This fixes all the remaining `max-len` infractions.
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue May 12, 2024
DEFRA/water-abstraction-team#115

Following on from [Fix all max-len ESLint rule infractions](#1001) this change fixes the remaining infractions based on our current rules (all 1 of them!)
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue May 12, 2024
DEFRA/water-abstraction-team#115

Following on from [Fix all max-len ESLint rule infractions](#1001) this change fixes the remaining infractions based on our current rules (all 1 of them!)
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue May 12, 2024
DEFRA/water-abstraction-team#115

We have a written convention that [all arrow functions should wrap params in parens even when there is only one](https://github.com/DEFRA/water-abstraction-team/blob/main/coding_conventions.md#arrow-functions).

We think code is clearer to follow and understand when it is _consistent_. JavaScript will allow you to skip adding them when there is only one param to an arrow function. But we think this inconsistency is not helpful to those new to the language or the team.

So, it is our convention to _always_ add parens. Now we are using ESLint we can make the tool highlight when this is missed rather than relying on reviewers.
Cruikshanks added a commit that referenced this issue May 12, 2024
#115

It has been an unwritten rule for some time now that we always add 'use strict' to the top of our JavaScript files. Why?

> JavaScript's strict mode is a way to opt in to a restricted variant of JavaScript, thereby implicitly opting-out of "sloppy mode".
> [MSDN docs - Strict mode](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode)

When the alternate mode is named 'sloppy', would you want to use it!?

The docs do provide some further explanation of what declaring strict mode means

> Strict mode makes several changes to normal JavaScript semantics:
>
> 1. Eliminates some JavaScript silent errors by changing them to throw errors.
> 2. Fixes mistakes that make it difficult for JavaScript engines to perform optimizations: strict mode code can sometimes be made to run faster than identical code that's not strict mode.
> 3. Prohibits some syntax likely to be defined in future versions of ECMAScript.

For these reasons we've had it as an unwritten convention to add it to all files for some time. This change is just to 'resolves' that state and make it a written convention!
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue May 12, 2024
DEFRA/water-abstraction-team#115

It is currently an unwritten convention (we are [working on](DEFRA/water-abstraction-team#117) fixing that!) to add 'use strict' to the top of all our files. Why?

> JavaScript's strict mode is a way to opt in to a restricted variant of JavaScript, thereby implicitly opting-out of "sloppy mode".
> [MSDN docs - Strict mode](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode)

When the alternate mode is named 'sloppy', would you want to use it!?

The docs do provide some further explanation of what declaring strict mode means

> Strict mode makes several changes to normal JavaScript semantics:
>
> 1. Eliminates some JavaScript silent errors by changing them to throw errors.
> 2. Fixes mistakes that make it difficult for JavaScript engines to perform optimizations: strict mode code can sometimes be made to run faster than identical code that's not strict mode.
> 3. Prohibits some syntax likely to be defined in future versions of ECMAScript.

For these reasons, this change updates our ESLint rules to ensure we do this.
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue May 12, 2024
DEFRA/water-abstraction-team#115

Just like [parens in arrow functions](#1004) how you write an [arrow function](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions) can differ.

```javascript
const materials = [
  'Hydrogen',
  'Helium',
  'Lithium',
  'Beryllium'
]

// Block body
materials.forEach((material) => {
  console.log(`${material} - ${material.length}`)
})

// Concise body
materials.forEach((material) => console.log(`${material} - ${material.length}`))
```

The _block-body_ version can be applied in all cases. But like with parens where devs will often drop the parens when there is only 1 param, they'll use _concise_ for simple arrow functions and _block-body_ for the rest.

To us though, the argument is the same. For those new or inexperienced with JavaScript this inconsistency can be confusing and a blocker to progressing with the language. The more we can do to keep the code base simple and consistent, the easier new folks will find working with it.

So, this change enables [the rule in ESLint](https://eslint.org/docs/latest/rules/arrow-body-style) that requires all arrow-functions to use the block-body.
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue May 12, 2024
DEFRA/water-abstraction-team#115

This has been an unwritten convention we often pick up in the PRs of those new to the team. We like to see a blank line before the `return` statement in a function.

```javascript
// Bad
function rubbishNameGenerator () {
  const firstName = 'John'
  const lastName = 'Doe'
  return `${firstName} ${lastName}`
}

// Good
function rubbishNameGenerator () {
  const firstName = 'John'
  const lastName = 'Doe'

  return `${firstName} ${lastName}`
}
```

But we don't want to see it all the time!

// Bad
function rubbishNameGeneratorV2 (useAlternate = false) {
  if (useAlternate) {

    return 'Jane Doe'
  }

  return 'John Doe'
}

// Good
function rubbishNameGeneratorV2 (useAlternate = false) {
  if (useAlternate) {
    return 'Jane Doe'
  }

  return 'John Doe'
}
```

We weren't sure we could make ESLint see the difference but it turns out we can. So, this change enables and adds an initial configuration for the rule [padding-line-between-statements](https://eslint.style/rules/js/padding-line-between-statements).

> We expect to expand the config to cover some more unwritten conventions regarding white space in our code. But those will come separately!
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue May 13, 2024
DEFRA/water-abstraction-team#115

We have a written convention that [all arrow functions should wrap params in parens even when there is only one](https://github.com/DEFRA/water-abstraction-team/blob/main/coding_conventions.md#arrow-functions).

We think code is clearer to follow and understand when it is _consistent_. JavaScript will allow you to skip adding them when there is only one param to an arrow function. But we think this inconsistency is not helpful to those new to the language or the team.

So, it is our convention to _always_ add parens. Now we are using ESLint we can make the tool highlight when this is missed rather than relying on reviewers.
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue May 13, 2024
DEFRA/water-abstraction-team#115

It is currently an unwritten convention (we are [working on](DEFRA/water-abstraction-team#117) fixing that!) to add 'use strict' to the top of all our files. Why?

> JavaScript's strict mode is a way to opt in to a restricted variant of JavaScript, thereby implicitly opting-out of "sloppy mode".
> [MSDN docs - Strict mode](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode)

When the alternate mode is named 'sloppy', would you want to use it!?

The docs do provide some further explanation of what declaring strict mode means

> Strict mode makes several changes to normal JavaScript semantics:
>
> 1. Eliminates some JavaScript silent errors by changing them to throw errors.
> 2. Fixes mistakes that make it difficult for JavaScript engines to perform optimizations: strict mode code can sometimes be made to run faster than identical code that's not strict mode.
> 3. Prohibits some syntax likely to be defined in future versions of ECMAScript.

For these reasons, this change updates our ESLint rules to ensure we do this.
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue May 13, 2024
DEFRA/water-abstraction-team#115

It is currently an unwritten convention (we are [working on](DEFRA/water-abstraction-team#117) fixing that!) to add 'use strict' to the top of all our files. Why?

> JavaScript's strict mode is a way to opt in to a restricted variant of JavaScript, thereby implicitly opting-out of "sloppy mode".
> [MSDN docs - Strict mode](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode)

When the alternate mode is named 'sloppy', would you want to use it!?

The docs do provide some further explanation of what declaring strict mode means

> Strict mode makes several changes to normal JavaScript semantics:
>
> 1. Eliminates some JavaScript silent errors by changing them to throw errors.
> 2. Fixes mistakes that make it difficult for JavaScript engines to perform optimizations: strict mode code can sometimes be made to run faster than identical code that's not strict mode.
> 3. Prohibits some syntax likely to be defined in future versions of ECMAScript.

For these reasons, this change updates our ESLint rules to ensure we do this.
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue May 13, 2024
DEFRA/water-abstraction-team#115

Just like [parens in arrow functions](#1004) how you write an [arrow function](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions) can differ.

```javascript
const materials = [
  'Hydrogen',
  'Helium',
  'Lithium',
  'Beryllium'
]

// Block body
materials.forEach((material) => {
  console.log(`${material} - ${material.length}`)
})

// Concise body
materials.forEach((material) => console.log(`${material} - ${material.length}`))
```

The _block-body_ version can be applied in all cases. But like with parens where devs will often drop the parens when there is only 1 param, they'll use _concise_ for simple arrow functions and _block-body_ for the rest.

To us though, the argument is the same. For those new or inexperienced with JavaScript this inconsistency can be confusing and a blocker to progressing with the language. The more we can do to keep the code base simple and consistent, the easier new folks will find working with it.

So, this change enables [the rule in ESLint](https://eslint.org/docs/latest/rules/arrow-body-style) that requires all arrow-functions to use the block-body.
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue May 13, 2024
DEFRA/water-abstraction-team#115

Just like [parens in arrow functions](#1004) how you write an [arrow function](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions) can differ.

```javascript
const materials = [
  'Hydrogen',
  'Helium',
  'Lithium',
  'Beryllium'
]

// Block body
materials.forEach((material) => {
  console.log(`${material} - ${material.length}`)
})

// Concise body
materials.forEach((material) => console.log(`${material} - ${material.length}`))
```

The _block-body_ version can be applied in all cases. But like with parens where devs will often drop the parens when there is only 1 param, they'll use _concise_ for simple arrow functions and _block-body_ for the rest.

To us though, the argument is the same. For those new or inexperienced with JavaScript, this inconsistency can be confusing and a blocker to progressing with the language. The more we can do to keep the code base simple and consistent, the easier new folks will find working with it.

So, this change enables [the rule in ESLint](https://eslint.org/docs/latest/rules/arrow-body-style) that requires all arrow functions to use the block-body.

---

The [ESLint stylistic plugin](https://eslint.style/) we use has a related rule; [https://eslint.style/rules/js/implicit-arrow-linebreak#when-not-to-use-it](https://eslint.style/rules/js/implicit-arrow-linebreak#when-not-to-use-it) which suggests it should not be used when `arrow-body-style` is set to 'always'. This is why it is disabled as part of this change.
Cruikshanks added a commit that referenced this issue May 13, 2024
#115

We have been working on using ESLint to help highlight coding convention infractions instead of folks either having to remember them when submitting or reviewing PRs. In [Ensure use of block body for arrow functions](DEFRA/water-abstraction-system#1006) we managed to figure out how to have the tool highlight when someone has used concise over block-body format for an arrow function.

However, the rule cannot distinguish between when it was being used normally, or just to check if something is 'truthy'. That was the one exception we had and there is no point in keeping it documented here if the tool won't allow. We also didn't think it was worth the effort to try and 'hack' ESLint or develop a custom rule just to support this. On balance, its goes against one of our guiding principle of consistency anyway.

So, this change removes the exceptions from our docs.
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue May 13, 2024
DEFRA/water-abstraction-team#115

This has been an unwritten convention we often pick up in the PRs of those new to the team. We like to see a blank line before the `return` statement in a function. We believe it helps make the code easier to follow and understand.

```javascript
// Bad
function rubbishNameGenerator () {
  const firstName = 'John'
  const lastName = 'Doe'
  return `${firstName} ${lastName}`
}

// Good
function rubbishNameGenerator () {
  const firstName = 'John'
  const lastName = 'Doe'

  return `${firstName} ${lastName}`
}
```

But we don't want to see it all the time!

```javascript
// Bad
function rubbishNameGeneratorV2 (useAlternate = false) {
  if (useAlternate) {

    return 'Jane Doe'
  }

  return 'John Doe'
}

// Good
function rubbishNameGeneratorV2 (useAlternate = false) {
  if (useAlternate) {
    return 'Jane Doe'
  }

  return 'John Doe'
}
```

We weren't sure we could make ESLint see the difference but it turns out we can! So, this change enables and adds an initial configuration for the rule [padding-line-between-statements](https://eslint.style/rules/js/padding-line-between-statements).

> We expect to expand the config to cover some more unwritten conventions regarding white space in our code. But those will come separately!
Cruikshanks added a commit that referenced this issue May 15, 2024
#115

We have been working on using ESLint to help highlight coding convention infractions instead of folks either having to remember them when submitting or reviewing PRs. In [Ensure use of block body for arrow functions](DEFRA/water-abstraction-system#1006) we managed to figure out how to have the tool highlight when someone has used concise over block-body format for an arrow function.

However, the rule cannot distinguish between when it was being used normally, or just to check if something is 'truthy'. That was the one exception we had and there is no point in keeping it documented here if the tool won't allow it. We also didn't think it was worth the effort to try and 'hack' ESLint or develop a custom rule just to support this. On balance, it goes against one of our guiding principles of consistency anyway.

So, this change removes the exceptions from our docs.
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue Jun 27, 2024
DEFRA/water-abstraction-team#115

We enhanced our linting rules to enforce [spaces around blocks and expressions](#1143). This is a great step to helping folks 'fall into the pit of success' and avoid reviewers having to nit-pick PRs.

But having enabled it we've spotted it is highlighting some things that are perfectly acceptable. We also think there are a few other options we could add.

So, this change is about building on the [padding-line-between-statements](https://eslint.style/rules/js/padding-line-between-statements) we've already added.
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue Jun 28, 2024
DEFRA/water-abstraction-team#115

We enhanced our linting rules to enforce [spaces around blocks and expressions](#1143). This is a great step to helping folks 'fall into the pit of success' and avoid reviewers having to nitpick PRs.

But having enabled it, we've spotted that it is highlighting some things that are perfectly acceptable.

We also think there are a few other options we could add.

So, this change is about building on the [padding-line-between-statements](https://eslint.style/rules/js/padding-line-between-statements) we've already added.

**Notes**

- Remove the config that is causing the violation
- Ensure blanks after 'use strict' declaration
- Ensure blanks after variable declarations

That last change ensures we put a blank line between variable declarations and the logic.

Running the linter across all files highlighted a number of violations. But the first 3 we fixed in this change give a good example of what to expect from this rule.

- `app/presenters/base.presenter.js` - simple example of separating variables from logic to help readability
- `app/presenters/bill-runs/two-part-tariff/review-bill-run.presenter.js` - demonstrating that comments are handled!
- `app/presenters/licences/set-up.presenter.js` - this is an example of compromise. It could be rightly argued it is better to have the var declaration next to the logic. But for the sake of consistency and having a tool to automate this stuff, we'll compromise and make the change
Cruikshanks pushed a commit to DEFRA/water-abstraction-system that referenced this issue Nov 22, 2024
DEFRA/water-abstraction-team#115

Now, we are a team of seven, with seven different opinions on the 'right' way to write the code! Because of our size, we are splitting into two teams to focus on various features simultaneously. But we'll still be working with the one code base.

We'd already moved from just using [StandardJS](https://standardjs.com/) to lint our code to using **StandardJS** via **ESLint** (we kept the rules but not the tool) because there are too many cases where **StandardJS** has no ruling, but we wanted one.

However, each time these rules don't provide a style convention, the team must stop, discuss, debate, and finally decide how something will be done. We want something else to take the decision away from us!

Step forward [Prettier](https://prettier.io/). **Prettier** is an opinionated code formatter that focuses only on the style of the code.

> Prettier enforces a consistent code style (i.e. code formatting that won’t affect the AST) across your entire codebase because it disregards the original styling by parsing it away and re-printing the parsed AST with its own rules that take the maximum line length into account, wrapping code when necessary.

So, any rules or conventions we have that would affect the [Abstract Syntax Tree (AST)](https://www.nearform.com/insights/what-is-an-abstract-syntax-tree/) would still be controlled by **ESlint**. These are the things as a team it is worth spending time debating and agreeing.

For the rest, we intend to let **Prettier** take over. It is widely used across the JavScript community and is popularly advocated for teams that become large or dispersed like ours.

Our approach to the adoption was first to remove our existing **ESlint** config, add **Prettier**, and then let it update all the files.

Then, having checked through as a team there were no 'red-line' changes, commit them. We then add **ESlint** back in and only re-apply the non-stylistic rules.

Our research found that JSDoc is still better managed through **ESlint** (**Prettier** does nothing with it), so we also added our original config for that.

Another fundamental change is that we are no longer bringing in **StandardJS** as an **ESlint** extension. This means we can move to the latest **ESlint** v9 and away from the [deprecated config file format](https://eslint.org/docs/latest/use/configure/configuration-files-deprecated) to the new [flat file config](https://eslint.org/docs/latest/use/configure/configuration-files).

Previously, we'd been blocked because extensions are not supported in ESLint v9. **StandardJS** does provide [eslint-config-standard](https://github.com/standard/eslint-config-standard), but along with the core project, it does not appear to be actively maintained, is still using the deprecated **ESLint** style rules and has an error so cannot be used.

> See [Switch to new eslint config format](#991) for more details on why this was blocking us

We'd resigned ourselves to manually copying and updating the rules from their plugin when we revisited an old issue when first switching to **ESLint** with **StandardJS**. [Support for Eslint v9 Flat Config format](standard/eslint-config-standard#411) was one of the blockers that meant to have both **ESLint** and **StandardJS** play ball, we were stuck on ESLint v8.

We spotted that there had been some [recent activity](standard/eslint-config-standard#411 (comment)), all of which referenced an alternative called [neostandard](https://github.com/neostandard/neostandard).

And oh gosh! All our dreams were answered.

- Built for ESlint to avoid the need for separate IDE tooling
- Built for the latest ESLint (v9), so flat-file config is supported
- Just like we did, any style rules have been updated to use @stylistic/eslint-plugin
- An explicit desire to work with current practices. So, built for use with ESLint only, banning or requiring `;` is now an option, and disabling style rules for those opting to use **prettier** is possible

For context, maintenance on **StandardJS** and related packages like **eslint-config-standard** has been stalled for some time. **neostandard** references the issue as being a [block in governance and direction of travel](standard/standard#1948 (comment)). I've not been through every message, but it appears the maintainers are split between those who remained committed to StandardJS's 'one-tool one-way' approach and those looking to move to where most folks are: **ESLint**.

Even our own @johnwatson484 [has gotten involved!](standard/standard#1948 (comment)).

We're betting this isn't going to be resolved any time soon, so to avoid us having to maintain standard rules in our ESLint config manually, the final fundamental change to highlight is we're now using ESLint + neostandard to manage coding rules.
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue Dec 24, 2024
DEFRA/water-abstraction-team#115

Recently, we [overhauled our project linting](#1476). We moved to ESLint v9 and its new flat file config, switched to Prettier to handle our 'style' conventions, and replaced StandardJS with Neostandard.

On that last point, the one rule we could not migrate at the time, was enforcement of `.js` extensions when requiring modules. We know Node.js will handle this for us: the extension is not required. But eventually we intend to migrate to ESM modules and from previous attempts, having the `.js` in place reduces the amount of work this requires.

We are now happy to report that in v0.12.0 [Neostandard](https://github.com/neostandard/neostandard) has adopted [eslint-plugin-import-x](https://github.com/un-ts/eslint-plugin-import-x) so we can add the rule back in, which is handy, as we've spotted new instances of folks forgetting to do this!
Cruikshanks added a commit to DEFRA/water-abstraction-system that referenced this issue Dec 30, 2024
DEFRA/water-abstraction-team#115

Recently, we [overhauled our project linting](#1476). We moved to ESLint v9 and its new flat file config, switched to Prettier to handle our 'style' conventions, and replaced StandardJS with Neostandard.

On that last point, the one rule we could not migrate at the time was the enforcement of `.js` extensions when requiring modules. We know Node.js will handle this for us and that the extension is not required. Eventually, we intend to migrate to ESM modules, and from previous attempts, having the `.js` in place reduces the amount of work this requires.

We are now happy to report that in v0.12.0 [Neostandard](https://github.com/neostandard/neostandard) has adopted [eslint-plugin-import-x](https://github.com/un-ts/eslint-plugin-import-x) so we can add the rule back in, which is handy, as we've spotted new instances of folks forgetting to do this!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant