GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||
GeezerVison is showing github.com — test: use destructuring on require by juanarbol · Pull Request #24455 · nodejs/node Conversation
|
|
Hi, @juanarbol! Welcome and thanks for the pull request! The change is fine, I guess, but it's not clear to me that this is a significant improvement over the existing code. Curious to get the opinion of other collaborators. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I took idea from Nodetodo, just make more clear an specific test. It's quite hard find anything to help |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
I'm not sure what you mean. There's nothing on the Node Todo website that suggests that introducing destructuring in a situation like this is desirable. Can you point me to where you got the suggestion?
I'll email you a suggested task. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Sure, I saw this commit in Nodetodo's list (Commits From NodeTodo Activities), it's kind of a code refactor of a particular unit test. I read some test files and found almost same "problem" in this particular file changed on this commit (mine), It's like an inspiration, I guess. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Ah, I see. That commit uses destructuring to replace assigning a property to a variable. This change, though, replaces a require() without a property being assigned. The former arguably improves readability. I'm not sure this has the same effect. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Resume CI: https://ci.nodejs.org/job/node-test-pull-request/18903/ |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Landed in e0893f0 Thanks for the contribution! 🎉 (If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
PR-URL: nodejs#24455 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: #24455 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: #24455 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: #24455 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: nodejs#24455 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: #24455 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: #24455 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.