Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

[WIP] Parallel lint test #4483

Closed
wants to merge 4 commits into from
Closed

[WIP] Parallel lint test #4483

wants to merge 4 commits into from

Conversation

timocov
Copy link
Contributor

@timocov timocov commented Jan 27, 2019

PR checklist

Overview of change:

It's the first version of parallel linting. See #2328 (comment).

The main changes are:

  1. Slight refactoring around runner (moved some interfaces to separate files, added/created ability to filter rules typed/non-typed, etc) and running linting (moved some code to src/doLinting.ts module, etc).
  2. Added new CLI option --parallel to run linter in parallel.

Is there anything you'd like reviewers to focus on?

  1. The whole PR 🙂
  2. Grammar (English is not my language).
  3. Files/classes/types names - I'm not familiar with tslint code base a lot and might misunderstood something.
  4. I've place TODOs in some places with comments
  5. How we can test it? 🤔

CHANGELOG.md entry:

@timocov
Copy link
Contributor Author

timocov commented Jan 27, 2019

In my local tests with running parallel linting for tslint code base I got decreasing lint time from 10 to 7 seconds on my PC with 2 workers. If I run with 8 workers (I have i7 with 4 cores/8 threads), the parallel run is a little bit slower (~1 second) - I guess that is because spawning the process isn't cheap and we need to provide users specify how much workers they want to have.

@timocov
Copy link
Contributor Author

timocov commented Apr 16, 2019

Hey @JoshuaKGoldberg, I don't know whether we're going to review/merge this, but I just merged master into the branch and resolved conflicts 🙂

@JoshuaKGoldberg
Copy link
Contributor

Thanks @timocov! It's still unclear what's the right course to take. /cc @adidahiya

@JoshuaKGoldberg
Copy link
Contributor

Ping @adidahiya - I think this would be a reasonable change to include in v6 (#4811) if documented as an 'experimental' option. If there are severe bugs with it, well, TSLint's going away anyway. Thoughts?

@JoshuaKGoldberg
Copy link
Contributor

@timocov I'm going to go ahead and close this PR. We have, on average, about half a person's worth of maintenance on the TSLint side (no full time maintainers), and we're facing delays just getting the breaking change issues merged in. It doesn't seem feasible to add this feature given how late we are in the ESLint migration process: #4534. 😢. If something were to go wrong we can't guarantee there'd be staffing to fix it and release that fix.

Still, to reiterate: the code change itself looks great and I think you did a fantastic job on it. You can always make a forked version of TSLint to use locally - extra features being added to TSLint isn't too big of a concern anymore.

Thanks, again for taking the time to work with us on this ❤️. Sorry it didn't work out ...!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants