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

refactor: lint and styling overhaul #77

Draft
wants to merge 87 commits into
base: staging
Choose a base branch
from

Conversation

ExampleWasTaken
Copy link
Contributor

@ExampleWasTaken ExampleWasTaken commented Aug 5, 2024

Overview

As extensively discussed, this PR overhauls all linting and styling rules used in this repository.

Goals

To recap the most important things, the main goals were

  • to remove the dependency structure of the current ESLint config,
  • separating linting and code formatting/styling,
  • match the flybywiresim/aircraft repository's linting and styling rules to keep code bases within FlyByWire as consistent as possible.

Implementation

These goals are achieved by:

  1. Introducing Prettier as formatter that enforces a consistent code style across many different file formats.
  2. Updating ESLint to v9 (latest version)
  3. Using typescript-eslint to add TypeScript-specific linting rules.
  4. Reducing specific rule configurations to a minimum and instead relying on industry-wide accepted rule-presets exposed through the recommended config-presets by ESLint and typescript-eslint.

Rule suggestions

Additionally to the recommended preset, I've gone through the available rules and suggest the following additional rules:

Rule Reasoning
no-await-in-loop Possible performance impact. See https://eslint.org/docs/latest/rules/no-await-in-loop
no-constructor-return Returning values from constructors is bad practice and can be confusing as constructors always return the object they instantiated (this).
no-self-compare Saves time during code review. See https://eslint.org/docs/latest/rules/no-self-compare
no-unreachable-loop Saves time during code review by preventing unnecessary one-time-loops. See https://eslint.org/docs/latest/rules/no-unreachable-loop
no-console (warn) Using winston should be preferred over direct console.log statements.

I think they are beneficial as they all save time during code reviews as they catch stuff that would most likely end up being questioned during a review anyway.

Additional Details

There are a few more things that are worth mentioning:

  • A new workflow is added to check for prettier errors whenever a PR is
    • opened
    • synchronized
    • reopened
    • marked as ready for review
  • To get even better type-specific linting, typescript-eslint is configured to use the recommendedTypeChecked config.
  • The ESLint Prettier plugin is used to report (and fix) Prettier errors as ESLint errors.
  • The ESLint Prettier config is used to turn off ESLint rules that are unnecessary or might conflict with Prettier.

Depends on

Soft-depends on

@ExampleWasTaken ExampleWasTaken marked this pull request as ready for review October 27, 2024 20:04
@ExampleWasTaken ExampleWasTaken marked this pull request as draft October 27, 2024 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant