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

Enforcing prettier & eslint #2378

Closed
mattjennings opened this issue Jun 29, 2022 · 2 comments
Closed

Enforcing prettier & eslint #2378

mattjennings opened this issue Jun 29, 2022 · 2 comments
Labels
stale This issue or PR has not had any activity recently

Comments

@mattjennings
Copy link
Contributor

mattjennings commented Jun 29, 2022

There is a prettier config file but it is not enforced. Running npm run pretty right now affects 230~ files and also causes some lint errors:

➜  Excalibur git:(fix/2376) ✗ npm run lint  

> excalibur@0.26.0 lint
> npm run eslint && npm run eslint:spec


> excalibur@0.26.0 eslint
> eslint "src/engine/**/*.ts"


/Users/mattjennings/dev/Excalibur/src/engine/Engine.ts
  771:15  error  Strings must use singlequote  quotes
  785:17  error  Strings must use singlequote  quotes

/Users/mattjennings/dev/Excalibur/src/engine/EntityComponentSystem/System.ts
  36:1  error  Expected indentation of 0 spaces but found 2                                   @typescript-eslint/indent
  37:1  error  Opening curly brace does not appear on the same line as controlling statement  brace-style

/Users/mattjennings/dev/Excalibur/src/engine/Graphics/Context/shader.ts
  30:1  error  Expected indentation of 4 spaces but found 2  @typescript-eslint/indent
  31:1  error  Expected indentation of 4 spaces but found 2  @typescript-eslint/indent

/Users/mattjennings/dev/Excalibur/src/engine/Graphics/Font.ts
  69:1  error  Expected indentation of 10 spaces but found 12  @typescript-eslint/indent
  70:1  error  Expected indentation of 10 spaces but found 12  @typescript-eslint/indent
  71:1  error  Expected indentation of 10 spaces but found 12  @typescript-eslint/indent
  72:1  error  Expected indentation of 8 spaces but found 10   @typescript-eslint/indent

@eonarheim suggested removing it altogether. I can make a PR to do that if that's the desire, but I'm a fan of prettier for formatting and linting should be enforced in some way too (which as far as I can tell is currently not the case, but correct me if I'm wrong).

What are your thoughts on setting up something like husky to enforce linting (and prettier if desired) with precommit hooks?

@github-actions
Copy link

This issue hasn't had any recent activity lately and is being marked as stale automatically.

@github-actions github-actions bot added the stale This issue or PR has not had any activity recently label Aug 29, 2022
eonarheim pushed a commit that referenced this issue Apr 8, 2024
See issue #2378 

- Add prettier for code formatting
- Update eslint config to parse other parts of codebase outside of `src/engine`, and also remove rules that are handled by prettier
- Add husky & lint-staged to enforce formatting on git commit
- Add format step to `all` and `all:ci` scripts

I separated the commit that runs the format & lint on all files into #3015 so the config can be easily reviewed here. 

Once we're happy with this, we can merge that PR into this one and ignore the commit(s) using `git blame --ignore-rev` to remove it from git blame history.
@eonarheim
Copy link
Member

@mattjennings Implemented! #3015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale This issue or PR has not had any activity recently
Projects
None yet
Development

No branches or pull requests

2 participants