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

test: replace anonymous function with arrow function #24528

Closed
wants to merge 1 commit into from
Closed

test: replace anonymous function with arrow function #24528

wants to merge 1 commit into from

Conversation

codegagan
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 20, 2018
@Trott
Copy link
Member

Trott commented Nov 20, 2018

While these kinds of commits are 👍 for first-contributions, I'm not sure they're of sufficient value to warrant a bunch of pull requests from existing contributors. I'd prefer people try to do something a bit more meaningful for subsequent commits. What do other Collaborators think?

(If we really want this type of thing to get done quickly instead of be a good exercise for new contributors, this is the type of thing that can be easily done with an ESLint rule and a --fix flag. )

@refack
Copy link
Contributor

refack commented Nov 20, 2018

Hello @codegagan,
Now that your a "second timer", may we ask you to batch these sort of changes together. Having multiple PR increases our (and I'm assuming your) overhead.

Also be aware that you might get push-back. I think that is only natural, as we do expect more and more as you become more of an experienced contributor.

If I may suggest, we have a large list of stalled Issues and PR, you could take a look at those. They are things that could benefit the project, but need a champion to push them over the line.

@gireeshpunathil gireeshpunathil added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 21, 2018
@gireeshpunathil
Copy link
Member

IIRC there were some code and learners whom we provided extra assignment slip(s) to be completed as home work as it was getting late in the evening on 17th and in case if they cannot do the first one due to its relatively complex nature (C++ code) - is this that category @codegagan ?

In either case, I concur to @Trott and @refack on the proposal on taking up bigger contribution ventures moving forward; at the same time feel happy to see @codegagan coming back to the repo. Wishing you great success with Node.js!

@gireeshpunathil
Copy link
Member

ok, now looking at the PR list I see there are 5 or 6 - so these are not my assignments but hand-picks . In a way it is good that you were able to search and find things of your own, but it makes sense to be contented with the list of arrow function assignments as of now. :)

@codegagan - one thing that comes to my mind is preparing C++ code (/src) for v8 deprecation. Talking to @ryzokuken this is what it is I think:

  • look at v8.h Search for methods with V8_DEPRECATE_SOON attributes
  • look around in the neighborhood comments to seek alternatives - methods with slightly differing params or return values.
  • search for usages of such deprecating methods in /src
  • replace those with recommended versions.

/cc @ryzokuken to confirm my understanding / providing more direct instructions. thanks!

@codegagan
Copy link
Contributor Author

@refack , @gireeshpunathil Thankyou for the guidance.

  • This is the case of extra assignment slips(actually a page) which I took home. I thought it important to push this task as thats what organizers would expect since I have taken the slips.

  • Reason for separate PRs instead of combined one: I clearly remember asking @thefourtheye at the event that why cannot we push all at once and he suggested that since it is unit of work and the good practice is to have separate. (Of course, it was overhead for me).

@gireeshpunathil regarding the v8 deprecation task, do you think it would be more appropriate to have instructions in an issue (than here) ? I can create one and paste these and then look into it.

@gireeshpunathil
Copy link
Member

make sense to me, thanks!

@gireeshpunathil
Copy link
Member

@danbev
Copy link
Contributor

danbev commented Nov 23, 2018

Re-run of failing node-test-commit-windows-fanned/.

@hiroppy hiroppy mentioned this pull request Nov 24, 2018
62 tasks
@gireeshpunathil
Copy link
Member

@gireeshpunathil
Copy link
Member

windows-fanned->vs17->test-trace-events-api-worker-disabled is a known failure, landing.

@gireeshpunathil
Copy link
Member

landed as db84fd2

gireeshpunathil pushed a commit that referenced this pull request Nov 25, 2018
PR-URL: #24528
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this pull request Nov 27, 2018
PR-URL: #24528
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
PR-URL: #24528
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
4 tasks
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
PR-URL: nodejs#24528
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 11, 2019
PR-URL: #24528
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Feb 12, 2019
rvagg pushed a commit that referenced this pull request Feb 28, 2019
PR-URL: #24528
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants