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

Conversation

@codegagan
Copy link
Contributor

commented Nov 20, 2018

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
@Trott

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

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

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2018

@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

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

make sense to me, thanks!

@gireeshpunathil

This comment has been minimized.

@danbev

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

@hiroppy hiroppy referenced this pull request Nov 24, 2018
50 of 62 tasks complete
@gireeshpunathil

This comment has been minimized.

@gireeshpunathil

This comment has been minimized.

Copy link
Member

commented Nov 25, 2018

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

@gireeshpunathil

This comment has been minimized.

Copy link
Member

commented Nov 25, 2018

landed as db84fd2

gireeshpunathil added a commit that referenced this pull request Nov 25, 2018
test: change anonymous function to arrow function
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 added a commit that referenced this pull request Nov 27, 2018
test: change anonymous function to arrow function
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 added a commit that referenced this pull request Nov 28, 2018
test: change anonymous function to arrow function
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 referenced this pull request Dec 5, 2018
4 of 4 tasks complete
refack added a commit to refack/node that referenced this pull request Jan 14, 2019
test: change anonymous function to arrow function
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 added a commit that referenced this pull request Feb 11, 2019
test: change anonymous function to arrow function
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 referenced this pull request Feb 12, 2019
rvagg added a commit that referenced this pull request Feb 28, 2019
test: change anonymous function to arrow function
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
Projects
None yet
9 participants
You can’t perform that action at this time.