Modernize code with a stricter lint config #111

fregante · 2020-09-25T07:11:04Z

Preview PR as described in #108 (comment). So far it's not very interesting, it's just eslint and Prettier cleaning things up.

Once that's merged, I'll revert the demo commit and re-fix the new code automatically.

A few more manual changes will also have to be applied on the next step:

❯ xo --fix

  test/pagination.js:3:1
  ⚠    3:1   test.todo() should be not be used.                                                                           ava/no-todo-test

  src/githubFactory.js:31:35
  ✖   31:35  Use the rest parameters instead of arguments.                                                                prefer-rest-params
  ✖   40:11  Use the spread operator instead of .apply().                                                                 prefer-spread
  ✖   40:34  Prefer await to then().                                                                                      promise/prefer-await-to-then
  ✖   63:37  Use the rest parameters instead of arguments.                                                                prefer-rest-params
  ✖   70:16  Use the spread operator instead of .apply().                                                                 prefer-spread
  ✖   70:39  Prefer await to then().                                                                                      promise/prefer-await-to-then
  ✖   92:20  Use the rest parameters instead of arguments.                                                                prefer-rest-params
  ✖   94:14  Use the spread operator instead of .apply().                                                                 prefer-spread
  ✖  103:10  Prefer await to then().                                                                                      promise/prefer-await-to-then
  ✖  106:10  Prefer await to then().                                                                                      promise/prefer-await-to-then
  ✖  113:37  Use the rest parameters instead of arguments.                                                                prefer-rest-params
  ✖  113:48  Prefer await to then().                                                                                      promise/prefer-await-to-then

  src/index.js:8:1
  ✖    8:1   Unexpected var, use let or const instead.                                                                    no-var
  ✖   34:6   Prefer await to then().                                                                                      promise/prefer-await-to-then
  ✖   82:17  Please rename the variable res. Suggested names are: response, result. A more descriptive name will do too.  unicorn/prevent-abbreviations
  ✖   93:6   Prefer await to then().                                                                                      promise/prefer-await-to-then

  test/simple.js:3:13
  ⚠  234:1   test.todo() should be not be used.                                                                           ava/no-todo-test
  ⚠  235:1   test.todo() should be not be used.                                                                           ava/no-todo-test
  ✖    3:13  Test files should not be imported.                                                                           ava/no-import-test-files
  ✖   98:5   t.end() should only be used inside of test.cb().                                                             ava/no-invalid-end

  test/progress.js:3:13
  ✖    3:13  Test files should not be imported.                                                                           ava/no-import-test-files
  ✖   27:9   No statements following a call to t.end().                                                                   ava/no-statement-after-end

  src/shouldDeleteFork.js:96:35
  ⚠    5:24  Function has too many parameters (5). Maximum allowed is 4.                                                  max-params
  ✖   96:35  Expected === and instead saw ==.                                                                             eqeqeq

  test/lib/index.js:79:15
  ✖   79:15  Unnecessary .call().                                                                                         no-useless-call

  test/filterUser.js:3:13
  ✖    3:13  Test files should not be imported.                                                                           ava/no-import-test-files

  src/bin.js:78:10
  ✖   78:10  length property should be compared to a value.                                                               unicorn/explicit-length-check

  4 warnings
  24 errors

package.json

fregante · 2020-09-25T08:54:03Z

Until now, all the changes only affected the style and not the behavior of the code.

The rest of the rules require (minimal) code behavior changes, so perhaps it's best to track them in a separate PR to avoid losing them in the sea of whitespace changes. This is the list:

❯ xo

  src/githubFactory.js:31:32
  ⚠   42:8   Avoid calling back inside of a promise.                                                                      promise/no-callback-in-promise
  ⚠   47:8   Avoid calling back inside of a promise.                                                                      promise/no-callback-in-promise
  ✖   31:32  Use the rest parameters instead of arguments.                                                                prefer-rest-params
  ✖   40:6   Expected catch() or return                                                                                   promise/catch-or-return
  ✖   40:6   Use the spread operator instead of .apply().                                                                 prefer-spread
  ✖   40:29  Prefer await to then().                                                                                      promise/prefer-await-to-then
  ✖   43:8   Each then() should return a value or throw                                                                   promise/always-return
  ✖   63:33  Use the rest parameters instead of arguments.                                                                prefer-rest-params
  ✖   70:12  Use the spread operator instead of .apply().                                                                 prefer-spread
  ✖   70:35  Prefer await to then().                                                                                      promise/prefer-await-to-then
  ✖   92:17  Use the rest parameters instead of arguments.                                                                prefer-rest-params
  ✖   94:11  Use the spread operator instead of .apply().                                                                 prefer-spread
  ✖  100:30  Use the rest parameters instead of arguments.                                                                prefer-rest-params

  src/index.js:8:1
  ✖    8:1   Unexpected var, use let or const instead.                                                                    no-var
  ✖   32:2   Expected catch() or return                                                                                   promise/catch-or-return
  ✖   34:4   Prefer await to then().                                                                                      promise/prefer-await-to-then
  ✖   41:4   Each then() should return a value or throw                                                                   promise/always-return
  ✖   82:12  Please rename the variable res. Suggested names are: response, result. A more descriptive name will do too.  unicorn/prevent-abbreviations
  ✖   93:4   Prefer await to then().                                                                                      promise/prefer-await-to-then

  test/simple.js:3:13                                                                           ava/no-todo-test
  ✖    3:13  Test files should not be imported.                                                                           ava/no-import-test-files
  ✖   98:3   t.end() should only be used inside of test.cb().                                                             ava/no-invalid-end

  test/progress.js:3:13
  ✖    3:13  Test files should not be imported.                                                                           ava/no-import-test-files
  ✖   27:5   No statements following a call to t.end().                                                                   ava/no-statement-after-end

  src/shouldDeleteFork.js:96:29
  ⚠    5:24  Function has too many parameters (5). Maximum allowed is 4.                                                  max-params
  ✖   96:29  Expected === and instead saw ==.                                                                             eqeqeq

  test/lib/index.js:79:8
  ✖   79:8   Unnecessary .call().                                                                                         no-useless-call

  test/filterUser.js:3:13
  ✖    3:13  Test files should not be imported.                                                                           ava/no-import-test-files

  src/bin.js:80:9
  ✖   80:9   length property should be compared to a value.                                                               unicorn/explicit-length-check

  6 warnings
  25 errors

I can temporarily disable these rules in this PR, and then fix them in a successive PR

https://lebab.unibtc.me/editor/

denis-sokolov · 2020-09-25T09:06:32Z

I agree with your reasoning!

fregante · 2020-09-25T09:08:02Z

Ok, I'll finish this up and then I'll work on the rest later

package.json

fregante

I tested this and it still works; I reviewed the code and it looks like all changes are stylistic only (i.e. the behavior didn't change), except the following

src/githubFactory.js

fregante · 2020-09-25T10:14:31Z

src/githubFactory.js

+	// Transform a github function call into a queued function call
+	// to ensure that only one API call runs at a time
+	// https://developer.github.com/guides/best-practices-for-integrators/#dealing-with-abuse-rate-limits
+	const qd = call => {


FYI I hope to replace this in a future PR; I think p-queue can replace both qd and the queue module while also helping a possible (internal) promisification of remove-github-forks. Callback hell can be avoided nowadays

I fully support that.

We’re on Node, so util.promisify can help transition a lot of these without rewriting everything in one go.

denis-sokolov

Thanks for the effort!

package.json

denis-sokolov · 2020-09-25T11:06:17Z

src/bin.js

-  console.log("  Create a token with permissions public_repo, delete_repo");
-  console.log("  Pass that in the CLI, enjoy!");
-  console.log("");
+program.on('--help', () => {


Here and in other similar places, is the function(){} syntax being replaced by () => {} automatically by xo or by you manually?

If automatically, I don’t mind much, just surprised to see. If manually by you, I don’t mind much, but I don’t see much benefit in that effort.

XO does this automatically with eslint’s prefer-arrow-callback but

For some reason it’s being ignored when run with Prettier (it’s not a conflicting style) prefer-arrow-callback is unset by Prettier xojs/xo#498 so I used Lebab to do the conversion until that bug is fixed

In many cases it’s shorter and cleaner, e.g.

a.map(function (b) { return b.data; }); // versus a.map(b => b.data);

Ah, prefer-arrow-callback, thanks for telling me. Let’s keep the xo default.

Of course they’re nice with short functions with implicit returns, but for longer functions, they do not save space, but add two more punctuation chars. ¯\_(ツ)_/¯

but for longer functions, they do not save space

I don't think that ever happens, do you have an example?

() => {return a} // Is always shorter than function () {return a}

The only difference is in declaration vs expression:

function a () {} const a = () => {}

for which I agree with you and hasn't been touched by this PR because I think there weren't any declarations.

We’re (presumably) not saving bytes, we’re saving visual noise that costs attention span. In that regard, list.map(item => item * 2) is by far better (has less visual noise) than list.map(function(item){ return item * 2; }). The same is not the case for multiline functions. In the example below, in my opinion, the version with the keyword is less visually noisy due to the keyword being more distinct and atomic than the ASCII art arrow:

addEventListener("click", function(event){ // ... }); addEventListener("click", (event) => { // ... });

This becomes increasingly problematic in very nested expressions due to the likelihood of the arrow syntax being more similar to other js operators and syntaxes:

foo <= bar ? quux / 3 : f(function(){ // perform operations }); foo <= bar ? quux / 3 : f(() => { // perform operations });

This is all subjective, so I don’t mean to speak the final word, just explaining what I mean.

I see what you mean. Many functions/callbacks will be gone in my promisification though, so it won't be much of a problem either way. ✌️

denis-sokolov · 2020-09-25T17:33:32Z

When you’re done with the branch, let me know, I’ll gladly rebase it. (I don’t prefer squashing unrelated changes together)

fregante · 2020-09-25T17:59:26Z

I'm done!

fregante · 2020-09-25T20:27:34Z

OT: I have great news. I promisified it partially and brought down the run time from 1m10s to 10s by using the native throttler

denis-sokolov · 2020-09-25T20:55:37Z

Ended up only having two commits due to the big dependencies between the various changes.

Thanks for this, modernizing this is beneficial for future maintenance!

Also, great news about the throttler! remove-github-forks was always a bit slowish, maybe that’ll change.

fregante commented Sep 25, 2020

View reviewed changes

package.json Show resolved Hide resolved

fregante mentioned this pull request Sep 25, 2020

Move from Travis to GitHub Actions #110

Merged

denis-sokolov reviewed Sep 25, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

fregante added 2 commits September 25, 2020 12:00

Modernize the code syntax using xo, Lebab, unicorn/prevent-abbreviations

533600d

https://lebab.unibtc.me/editor/

Switch prepublishOnly to prepack

1c352ee

fregante commented Sep 25, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

fregante commented Sep 25, 2020

View reviewed changes

src/githubFactory.js Outdated Show resolved Hide resolved

fregante marked this pull request as ready for review September 25, 2020 10:00

fregante commented Sep 25, 2020

View reviewed changes

denis-sokolov approved these changes Sep 25, 2020

View reviewed changes

fregante mentioned this pull request Sep 25, 2020

Promisify internals #112

Merged

denis-sokolov merged commit c4bc7e6 into denis-sokolov:master Sep 25, 2020

fregante deleted the lint branch September 25, 2020 20:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernize code with a stricter lint config #111

Modernize code with a stricter lint config #111

fregante commented Sep 25, 2020 •

edited

Loading

fregante commented Sep 25, 2020

denis-sokolov commented Sep 25, 2020

fregante commented Sep 25, 2020

fregante left a comment •

edited

Loading

fregante Sep 25, 2020

denis-sokolov Sep 25, 2020

denis-sokolov Sep 25, 2020

denis-sokolov left a comment

denis-sokolov Sep 25, 2020

fregante Sep 25, 2020

denis-sokolov Sep 25, 2020

fregante Sep 25, 2020 •

edited

Loading

denis-sokolov Sep 25, 2020

fregante Sep 25, 2020

denis-sokolov commented Sep 25, 2020

fregante commented Sep 25, 2020

fregante commented Sep 25, 2020

denis-sokolov commented Sep 25, 2020

Modernize code with a stricter lint config #111

Modernize code with a stricter lint config #111

Conversation

fregante commented Sep 25, 2020 • edited Loading

fregante commented Sep 25, 2020

denis-sokolov commented Sep 25, 2020

fregante commented Sep 25, 2020

fregante left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denis-sokolov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante Sep 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denis-sokolov commented Sep 25, 2020

fregante commented Sep 25, 2020

fregante commented Sep 25, 2020

denis-sokolov commented Sep 25, 2020

fregante commented Sep 25, 2020 •

edited

Loading

fregante left a comment •

edited

Loading

fregante Sep 25, 2020 •

edited

Loading