Maintenance: automate linting/formatting with Prettier + eslint

[dreamorosi]

Summary

At the moment the project uses eslint to lint and apply linting and code style rules across the project.

While this has helped immensely, as the codebase and number of contributors grow, it's time to enforce automated formatting so that styling and other rules are applied automatically.

A good candidate for this could be Prettier.

Why is this needed?

To enforce styling conventions and format the code automatically before it's committed/pushed.

Which area does this relate to?

Automation

Solution

No response

Acknowledgment

• This request meets Lambda Powertools Tenets
• Should this be considered in other Lambda Powertools languages? i.e. Python, Java, and .NET

Future readers

Please react with 👍 and your use case to help us understand customer demand.

[dreamorosi]

I have started working on this and have modified the .eslintrc that is present in the repo with the one below:

module.exports = {
  root: true,
  env: {
    browser: false,
    es2020: true,
    jest: true,
    node: true,
  },
  extends: [
    'plugin:@typescript-eslint/recommended',
    'plugin:prettier/recommended',
  ],
  parser: '@typescript-eslint/parser',
  plugins: ['@typescript-eslint', 'prettier'],
  settings: {
    'import/resolver': {
      node: {},
      typescript: {
        project: './tsconfig.json',
        alwaysTryTypes: true,
      },
    },
  },
  rules: {
    '@typescript-eslint/explicit-function-return-type': [
      'error',
      { allowExpressions: true },
    ], // Enforce return type definitions for functions
    '@typescript-eslint/explicit-member-accessibility': 'error', // Enforce explicit accessibility modifiers on class properties and methods (public, private, protected)
    '@typescript-eslint/member-ordering': [
      // Standardize the order of class members
      'error',
      {
        default: {
          memberTypes: [
            'signature',
            'public-field',
            'protected-field',
            'private-field',
            'constructor',
            'public-method',
            'protected-method',
            'private-method',
          ],
          order: 'alphabetically',
        },
      },
    ],
    '@typescript-eslint/no-explicit-any': 'error', // Disallow usage of the any type
    '@typescript-eslint/no-unused-vars': ['error', { argsIgnorePattern: '^_' }], // Disallow unused variables, except for variables starting with an underscore
    '@typescript-eslint/no-use-before-define': ['off'], // Check if this rule is needed
    'no-unused-vars': 'off', // Disable eslint core rule, since it's replaced by @typescript-eslint/no-unused-vars
    // Rules from eslint core https://eslint.org/docs/latest/rules/
    'array-bracket-spacing': ['error', 'never'], // Disallow spaces inside of array brackets
    'computed-property-spacing': ['error', 'never'], // Disallow spaces inside of computed properties
    'func-style': ['warn', 'expression'], // Enforce function expressions instead of function declarations
    'keyword-spacing': 'error', // Enforce spaces after keywords and before parenthesis, e.g. if (condition) instead of if(condition)
    'padding-line-between-statements': [
      // Require an empty line before return statements
      'error',
      { blankLine: 'always', prev: '*', next: 'return' },
    ],
    'no-console': 0, // Allow console.log statements
    'no-multi-spaces': ['error', { ignoreEOLComments: false }], // Disallow multiple spaces except for comments
    'no-multiple-empty-lines': ['error', { max: 1, maxBOF: 0, maxEOF: 0 }], // Enforce no empty line at the beginning & end of files and max 1 empty line between consecutive statements
    'no-throw-literal': 'error', // Disallow throwing literals as exceptions, e.g. throw 'error' instead of throw new Error('error')
    'object-curly-spacing': ['error', 'always'], // Enforce spaces inside of curly braces in objects
    'prefer-arrow-callback': 'error', // Enforce arrow functions instead of anonymous functions for callbacks
    quotes: ['error', 'single', { allowTemplateLiterals: true }], // Enforce single quotes except for template strings
    semi: ['error', 'always'], // Require semicolons instead of ASI (automatic semicolon insertion) at the end of statements
  },
};

Aside from adding comments to all rules to explain what they do (full list of rules here), I have added Prettier following the recommended config:

• added prettier in the plugins section
• added prettier/recommended in the extends section

as well as removed the following rules:

• @typescript-eslint/ban-ts-ignore (deprecated)
• @typescript-eslint/camelcase (deprecated)
• @typescript-eslint/member-delimiter-style' (deprecated)
• @typescript-eslint/no-inferrable-types (deprecated)
• @typescript-eslint/indent - now covered by Prettier
• arrow-body-style - value set was already default

[dreamorosi]

The new config produces a huge diff as it touches almost all code-related files in one way or another.

To avoid completely breaking the commit history and render git blame useless, my proposal would be to use the technique described here, which involves creating & committing a file called .git-blame-ignore-revs that will contain the hashes of the commits that we want to ignore, i.e.:

# .git-blame-ignore-revs
# Removed semi-colons from the entire codebase
a8940f7fbddf7fad9d7d50014d4e8d46baf30592
# Converted all JavaScript to TypeScript
69d029cec8337c616552756310748c4a507bd75a

This setting is supported both on GitHub and locally by setting git config blame.ignoreRevsFile .git-blame-ignore-revs either globally or with the --local flag.

Since we squash & merge when merging a PR, the hash to ignore would be only the one of the merge commit. This means also that we will need to add this file in a subsequent commit.

[dreamorosi]

The resulting PR (linked above) was way too big to be merged in one go.

I'm going to make the change in three waves:

1. Create one PR to add prettier and its config to the root of the project but leaving the original .eslintrc.js at the root
2. Open one PR for each section of the project (i.e. packages/metrics, packages/logger, layers, etc.) creating the new .eslintrc.js config & fixing the linting problems
3. When all the PRs above are merged, create one last PR to remove the local .eslintrc.js files and instead apply the new config at the root

This strategy relies on ESLint config hierarchy that allows to have multiple config files and gives precedence to the one closer to the code. In this case, the stricter config will be applied locally and gradually minimizing the PR diffs and potential blast radius for breaking things.

cc @am29d