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

Moving fully to eslint, removing prettier #1024

Merged
merged 49 commits into from
Aug 27, 2021
Merged

Moving fully to eslint, removing prettier #1024

merged 49 commits into from
Aug 27, 2021

Conversation

filmaj
Copy link
Contributor

@filmaj filmaj commented Jul 29, 2021

🙌 All of these changes are courtesy of @aoberoi and the work they put into #669 🙇

Summary

This PR is the beginning of a journey to tackle #842. The goals are:

  • 😌 when in doubt, formalize the existing style in the project, in order to avoid overchoice (being overwhelmed with too many options)
  • 🔪 remove prettier
  • 🥗 use a single eslint configuration for both project and test code
  • 👪 get buy-in from the team / project maintainers on the core testing / linting tools and configurations that we use on a daily basis to make our work more enjoyable and productive

This pull request fixes #842

Progress

Using this PR and running npm run lint currently yields:

✖ 189 problems (189 warnings)

🤓 Process

Tackling #842 is a big task! It will require many changes to many files across the project. As such, I propose we employ a divide-and-conquer approach consisting of many pull requests that would look something like the below. This is only a suggestion and I welcome feedback on the approach and am open to other alternatives!

  • First, we can review and discuss just the configuration changes on their own - just the changes encapsulated in this initial pull request. Are some linting and/or compilation rules silly, unwieldy or we just plain don't like them? Are there some favourite linting rules that we are missing that you believe we should include? This initial configuration review is rather "abstract" in the sense that it is divorced from seeing the impact of the configuration changes in project source code. However, that comes next!
  • Next, we can issue additional pull requests, using this tslint-to-eslint branch as a base, to offer suggestions on how to change the configurations / linting rules. These config/lint rule change PRs may be accompanied with the appropriate source code changes enabling project source to pass the rules in question. I think this offers an experimental and "let's try it out and see!" approach to figuring out which linting rules we want to include and which ones we want to move away from. With each such micro-PR merged into this PR, this base PR should in theory come closer and closer to passing the CI, as each PR should remove some number of linting errors.
  • Finally, once one or more micro-PRs have been merged into this PR and the CI passes, we can merge this PR into main.

📝 TODO

Just keeping a running track of outstanding tasks here - this section will likely be edited many times as this PR is worked on.

Requirements (place an x in each [ ])

@filmaj
Copy link
Contributor Author

filmaj commented Jul 29, 2021

I've created on such micro-PR, addressing the naming convention of type properties for source code located under src/types here: #1025. I hope this is helpful to demonstrate the kind of process/approach I suggested in tackling this task. Let me know what you all think!

EDIT: This has now been merged in.

@filmaj filmaj added discussion M-T: An issue where more input is needed to reach a decision semver:patch tests M-T: Testing work only and removed untriaged labels Jul 30, 2021
.eslintrc.js Show resolved Hide resolved
.eslintrc.js Show resolved Hide resolved
@filmaj
Copy link
Contributor Author

filmaj commented Aug 5, 2021

I have and will be issuing several more pull requests (I list them all in under the TODO section of the original post in this PR). I am trying to group the PRs by rule - one PR per rule, basically. I have merged the ones that do not change any existing rules and just adhere to relatively common / well-accepted rules already - but that does not mean those cannot be easily reverted or modified. I have also left the PRs that change lint rules open for review for people. In either case: all open/closed/merged PRs listed under the TODO section can be reviewed at the team's leisure, and comments can be left there.

Once I have created PRs for all rules that are failing, I will comment again.

… = operator, and prefix for other operators to be at the end of the line.
…s the use of short-circuit logical operators (|| and &&).
Allow use of "as Type" syntax for type assertions in test code.

Dont force symbol descriptors in test code
…he accessibility of constructors for various classes.
@filmaj
Copy link
Contributor Author

filmaj commented Aug 27, 2021

Alright, with @srajiang resolving the update to TS issues, I have now rebased this PR with the latest main. Let's see what shakes out from CI!

EDIT: FYI we will get this warning for now I suppose? Linting and tests still pass fine regardless.

WARNING: You are currently running a version of TypeScript which is not officially supported by @typescript-eslint/typescript-estree.

You may find that it works just fine, or you may not.

SUPPORTED TYPESCRIPT VERSIONS: >=3.3.1 <4.4.0

YOUR TYPESCRIPT VERSION: 4.4.2

Please only submit bug reports when using the officially supported version.

@filmaj filmaj merged commit 926b669 into main Aug 27, 2021
@filmaj filmaj deleted the tslint-to-eslint branch August 27, 2021 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion M-T: An issue where more input is needed to reach a decision semver:patch tests M-T: Testing work only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate ESLint to new linting configration
4 participants