Skip to content

Conversation

@fzuleta
Copy link

@fzuleta fzuleta commented Aug 26, 2022

Summary

  • Created .eslintrc to match previous rules
  • Made eslint/prettier work together
  • ran lint on src, ts_src and test
  • made sure all existing tests pass

This is rather a monster PR, let me know, happy to iterate on it, or if there's no interest it's ok to close it too.

@grunklejp
Copy link

You will likely want to match the current tslint config rules ("arrow-parens": false for example) to keep the diff as small as possible. Then make subsequent PR's to add whatever linting rule changes you'd like.

Concept ACK

@junderw
Copy link
Member

junderw commented Sep 1, 2022

You will likely want to match the current tslint config rules ("arrow-parens": false for example) to keep the diff as small as possible. Then make subsequent PR's to add whatever linting rule changes you'd like.

I agree with John here.

  1. Concept ACK
  2. I want to merge and release taproot first (so that @motorina0 won't have to deal with all the merge conflicts)

@motorina0
Copy link
Member

motorina0 commented Sep 2, 2022 via email

@fzuleta
Copy link
Author

fzuleta commented Sep 2, 2022

sounds good, I'll wait for taproot merge,

arrow-parens

funny enough 😅 , this one caused an interesting conflict between es-lint and prettier, since prettier recommends here to have it enabled.

But I absolutely understand the impact, will iterate on it to minimize risk

@junderw
Copy link
Member

junderw commented Nov 29, 2022

Sorry for the big diff!

Merged taproot. Please rebase and I'll merge this. Thanks!

- installed eslint integrations
- removed tslint
- added rules to match old tslint rules
- ran linter on ts_src and tests
@fzuleta
Copy link
Author

fzuleta commented Dec 5, 2022

hi @junderw did a rebase and pushed, here's a few notes since it's still affecting so many files

  • With ESlint rules I could pretty much mirror old tslint rules.
  • When applying Prettier rules ontop of eslint, I could not mirror them 😞 , since Prettier is highly opinionated, things like multiple imports/exports per line, or extra spaces [..] vs [....] throw errors and Prettier devs didnt give us a disable flag, so I had to jump in and fix formatting on a few files.

let me know what you think, Im happy to continue iterating on it

edit: FYI, I also updated the package-lock deps after upgrading prettier and installing eslint, and did run snyk test on the project locally with no vulnerabilities found

Copy link
Member

@junderw junderw left a comment

Choose a reason for hiding this comment

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

LGTM

@junderw junderw merged commit 9ae2089 into bitcoinjs:master Dec 6, 2022
@fzuleta fzuleta deleted the feature/es-lint branch December 6, 2022 14: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.

4 participants