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

Lint - spaces around blocks and expressions #1143

Merged
merged 1 commit into from
Jun 27, 2024

Conversation

jonathangoulding
Copy link
Collaborator

@jonathangoulding jonathangoulding commented Jun 26, 2024

This change should enforce linting on commons NITs picked up in pull requests.

6 Rules have been added.

  1. blankLine: 'always', prev: '*', next: 'block': Requires a blank line before a block statement ({}).

  2. blankLine: 'always', prev: '*', next: 'expression': Requires a blank line before an expression statement.

  3. blankLine: 'always', prev: 'block', next: '*': Requires a blank line after a block statement ({}).

  4. blankLine: 'always', prev: 'block', next: 'function': Requires a blank line after a block statement ({}) followed by a function declaration.

  5. blankLine: 'always', prev: 'expression', next: '*': Requires a blank line after an expression statement.

  6. blankLine: 'always', prev: 'function', next: '*': Requires a blank line after a function declaration.

This combination should also enforce the blank space after a describe block and a space before an expect expression in the test files.

https://github.com/DEFRA/water-abstraction-team/blob/main/coding_conventions.md

This change should enforce linting on commons NITs picked up in pull requests.

6 Rules have been added.

1. blankLine: 'always', prev: '*', next: 'block': Requires a blank line before a block statement ({}).

2. blankLine: 'always', prev: '*', next: 'expression': Requires a blank line before an expression statement.

3. blankLine: 'always', prev: 'block', next: '*': Requires a blank line after a block statement ({}).

4: blankLine: 'always', prev: 'block', next: 'function': Requires a blank line after a block statement ({}) followed by a function declaration.

5. blankLine: 'always', prev: 'expression', next: '*': Requires a blank line after an expression statement.

6. blankLine: 'always', prev: 'function', next: '*': Requires a blank line after a function declaration.

This combination should also enforce the blank space after a describe block and a space before an expect expression in the test files.

https://github.com/DEFRA/water-abstraction-team/blob/main/coding_conventions.md
@jonathangoulding jonathangoulding added the conventions Changes to tools that enforce our conventions label Jun 26, 2024
@jonathangoulding jonathangoulding self-assigned this Jun 26, 2024
@jonathangoulding jonathangoulding changed the title Lint - spaces around blocks Lint - spaces around blocks and expressions Jun 26, 2024
Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonathangoulding jonathangoulding merged commit 0cfd7ef into main Jun 27, 2024
6 checks passed
@jonathangoulding jonathangoulding deleted the chore-add-lint-rule-for-spacing branch June 27, 2024 10:56
Cruikshanks added a commit that referenced this pull request 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 that referenced this pull request 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conventions Changes to tools that enforce our conventions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants