Mentors for Code + Learn at Node Interactive 2017

@addaleax

/cc @nodejs/collaborators

o/ Will show up, I promise

On Sep 9, 2017 6:58 PM, "Rich Trott" ***@***.***> wrote: - @addaleax <https://github.com/addaleax> - @Trott <https://github.com/trott> - You? - You? - You? /cc @nodejs/collaborators <https://github.com/orgs/nodejs/teams/collaborators> — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#70>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV_3JwWrD2WJ0iXDxR12rv7L_se9Nks5sgsPBgaJpZM4PSDU6> .

Code + Learn will begin at 9 AM on Friday October 6, one day after the main conference is over. It will run until 12:30 PM. There will be an hour for lunch before the Collaborators Summit starts after that.

I'll be there! 💪

I will be there.

I will be there

I will be there.

Me too

I will be there

I'll be there!

I'm in!

I'll be there as well

where is it?

@XadillaX

@XadillaX Vancouver, Canada (4-6 Oct.) http://events.linuxfoundation.org/events/node-interactive/

I'll be there.

@hackygolucky

@hackygolucky What's the capacity of the room? (Trying to plan for maximum number of attendees.)

Count me in as well

I'll be there

…

On Thu, Oct 5, 2017 at 4:05 PM Lance Ball ***@***.***> wrote: Count me in as well — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#70 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAE0qY9wMVz-MaOZL78NVpa7zUOpSkiuks5spWDPgaJpZM4PSDU6> .

I'm working through some of this here:

nodejs/node#15864

And I've run into a discrepancy I'd like to understand re: the New method.

In v8.h, for the Date object, things look like so:

  static V8_DEPRECATE_SOON("Use maybe version.",
                           Local<Value> New(Isolate* isolate, double time));
  static V8_WARN_UNUSED_RESULT MaybeLocal<Value> New(Local<Context> context,
                                                     double time);

So we want to use the maybe version, which takes a context rather than an isolate. OK.

But for NumberObject, we have:

  static Local<Value> New(Isolate* isolate, double value);

And there's no version that takes a context.

This makes me wonder if this change is fully baked and ready to roll out to all the New invocations; or, alternatively, if there is some good rule of thumb by which I might understand why some now take a context by preference and some still are happy with an isolate and no use of Maybe.

Thanks!

@ofrobots

@ofrobots came by and explained that some of the factory/constructor methods can never fail and thus don't need the change.

I didn't think I would be a suitable mentor so I didn't show up...

Some factory methods do not throw and cannot fail, yes. Some API functions take a context because they might throw, and we need to construct an Error from the given context.

I'm not at NINA but have reviewed a few of the pull requests that have been raised -- Should nits like commit messages (subsystem, line lengths, etc.) be called out for first time contributors?

Also for reviewers I'm seeing similar issues with the changes to use common/fixtures where I've been recommending to use fixtures.path/fixtures.readKey/fixtures.readSync where appropriate.

I'm not at NINA but have reviewed a few of the pull requests that have been raised -- Should nits like commit messages (subsystem, line lengths, etc.) be called out for first time contributors?

I'd personally say asking new contributors to commit --amend and push --force is likely to be more pain than it's worth. I'd just leave a comment on landing saying that you changed the commit message to whatever, and maybe they could check out the commit message guidelines next time.

Also for reviewers I'm seeing similar issues with the changes to use common/fixtures where I've been recommending to use fixtures.path/fixtures.readKey/fixtures.readSync where appropriate.

I think that is something we should ask people to change. I've been using Comment rather than Request Changes, but I think either is fine.

I think it's worth specifically requesting they push another commit to their branch, otherwise we get loads of people opening new PRs whenever they make changes.

@nodejs/collaborators if you left a MacBook USB-C charger in the venue, Cassandra has it, and she'll be around tomorrow if you would like it back!

@Trott

@Trott thanks for organizing this. It was a lot of fun and such a great feeling to enable so many people at one time!

I'm noticing a number of the pull requests were closed by the authors for minor things such as not following the commit guidelines, or for no clear reason at all. Here is an example. It's a shame that the work in these PRs won't land. Does it make sense to reopen them? Or just let them go?

I assume most people closing PRs are doing it either by mistake, or because they think you have to open a new PR if you change something.

The first can hopefully be fixed by asking if they meant to close the PR, the second can be mitigated by saying "please fix by pushing another commit to your branch" if you request changes.

It's possible people close them because they don't want to deal with feedback, but I think/hope that will be less common.

220+ pull requests later...thanks, everyone! Closing...

I've been assigning issues to myself so I can land them in bulk. For anything with a Green CI I'm just doing:

Test all files changed in the last commit:

git show --pretty="" --name-only | xargs tools/test.py && make lint-js && core-validate-commit HEAD

Test all files changed since upstream branch (e.g. `upstream/master`):

Useful for multiple commits.

git diff-tree --no-commit-id --name-only -r `git rev-parse --abbrev-ref --symbolic-full-name @{u}` HEAD | xargs tools/test.py && 
make lint-js &&
git rev-list `git rev-parse --abbrev-ref --symbolic-full-name @{u}`...HEAD | xargs core-validate-commit

Mostly leaving these here so I can find them again in the future, but they might be useful for someone else.

Could everyone who's actively landing stuff please self-assign, so we don't do double work.

Navigation Menu

Repository navigation

Mentors for Code + Learn at Node Interactive 2017 #70

18 remaining items

Test all files changed in the last commit:

Test all files changed since upstream branch (e.g. `upstream/master`):

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Mentors for Code + Learn at Node Interactive 2017 #70

Description

Activity

MylesBorins commented on Sep 9, 2017

Trott commented on Sep 9, 2017

Qard commented on Sep 9, 2017

jasnell commented on Sep 9, 2017

targos commented on Sep 9, 2017

sam-github commented on Sep 9, 2017

gibfahn commented on Sep 9, 2017

tniessen commented on Sep 9, 2017

TimothyGu commented on Sep 10, 2017

hybrist commented on Sep 10, 2017

joyeecheung commented on Sep 10, 2017

XadillaX commented on Sep 10, 2017

joyeecheung commented on Sep 10, 2017

mhdawson commented on Sep 11, 2017

Trott commented on Sep 22, 2017

18 remaining items

lance commented on Oct 5, 2017

ofrobots commented on Oct 5, 2017

boutell commented on Oct 6, 2017

boutell commented on Oct 6, 2017

hashseed commented on Oct 6, 2017

richardlau commented on Oct 6, 2017

gibfahn commented on Oct 6, 2017

gibfahn commented on Oct 7, 2017

lance commented on Oct 7, 2017

lance commented on Oct 7, 2017

gibfahn commented on Oct 7, 2017

Trott commented on Oct 7, 2017

gibfahn commented on Oct 8, 2017

Test all files changed in the last commit:

Test all files changed since upstream branch (e.g. upstream/master):

refack commented on Oct 8, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Test all files changed since upstream branch (e.g. `upstream/master`):