search

grncdr / merge-stream

Merge multiple streams into one interleaved stream
MIT License
214 stars 16 forks source link

Modernize js to es6 #39

Closed dpilafian closed 4 years ago

dpilafian commented 5 years ago
  1. Set strict to "implied" (updated this to global)
  2. var to const or let
  3. functions to fat arrows (=>) and had to move definitions before usage
  4. Bumped testing timeout from 1000 to 1500 to allow completion
  5. Updated .jshintrc and added npm pretest script to do linting

This PR changes a lot of lines of code, but the changes are intended to be just syntactic updates. There should be no functional differences at all. The code should now look more modern but still behave exactly the same. I attempted to follow the existing styles in the project (such as indentation and semicolon usage) as much as possible, but feel free to reformat any style I didn't get right.

grncdr commented 5 years ago

I don't want to discourage contribution, but I don't like this PR for the following reasons:

  1. Unrelated changes combined into a single commit (the changes to config files are a separate concern from the code changes).
  2. It accidentally breaks the API.
  3. No explanation for why to make the change is given.
  4. It changes and re-orders nearly every line of code, which combined with points 1 and point 3 makes reviewing it feel like a chore.

I'll defer to the other maintainers if they want to fix the second point and merge anyways, but IMO this PR should be rejected.