Mentors for Code + Learn at Node Interactive 2017

[Trott]

• [x] @addaleax
• [x] @Trott
• [x] @MylesBorins
• [x] @Qard
• [x] @jasnell
• [x] @targos
• [x] @sam-github
• [x] @gibfahn
• [x] @joyeecheung
• [x] @tniessen
• [x] @TimothyGu
• [x] @jkrems
• [x] @mhdawson
• [x] @bengl
• [x] @BridgeAR
• [x] @mcollina
• [x] @Fishrock123
• [x] @lance
• [x] @ofrobots
• [ ] You?
• [ ] You?
• [ ] You?

/cc @nodejs/collaborators

[MylesBorins]

o/

Will show up, I promise

On Sep 9, 2017 6:58 PM, "Rich Trott" notifications@github.com 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 https://github.com/nodejs/code-and-learn/issues/70, or mute the thread https://github.com/notifications/unsubscribe-auth/AAecV_3JwWrD2WJ0iXDxR12rv7L_se9Nks5sgsPBgaJpZM4PSDU6 .

[Trott]

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.

[Qard]

I'll be there! 💪

[jasnell]

I will be there.

[targos]

I will be there

[sam-github]

I will be there.

[gibfahn]

Me too

[tniessen]

I will be there

[TimothyGu]

I'll be there!

[jkrems]

I'm in!

[joyeecheung]

I'll be there as well

[XadillaX]

where is it?

[joyeecheung]

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

[mhdawson]

I'll be there.

[Trott]

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

[Trott]

So, something I'd like to see happen in the days leading up to Code + Learn and maybe have one or two active people watching for on the day of: Let's get the CI to green. @nodejs/build @nodejs/testing Any volunteers to aggressively pursue build and test issues as they pop up in the next few weeks? Any @nodejs/build volunteers to be at the ready to fix any problematic CI hosts during the event?

[bengl]

I'm in.

[refack]

You know I am 💂

[BridgeAR]

I will be there as well

[Trott]

I've got 156 tasks so far. @hackygolucky What's the capacity of the room? Trying to figure out if that will be enough or if I need to pull together some more.

[mcollina]

I’ll be there

[Trott]

I have 157 first-time-contribution tasks.

The room holds 350.

Organizers said there are currently 300 people signed up.

I'll try to come up with another <gulp> 143 tasks today. If anyone has suggestions that will generate at least a few dozen tasks, let me know.

Whee!!!!

[bnoordhuis]

I don't know if it's well-suited for first-time contributors, but updating on a per-file basis files in src/ to use the new V8 APIs that take a Local<Context> (e.g. ToString(), ToObject(), Object::Get() and Object::Set(), etc.) would give you an additional ~60 tasks.

If @addaleax is there, she can help explain the code patterns.

[Trott]

@bnoordhuis Great, thanks. We can ask at the beginning if there are people who are comfortable with C++ and if they might want their first contribution to be a (simple, I assume) C++ change.

[Trott]

@bnoordhuis @addaleax Is there anything I can grep for that would give me the list of 60 files that could potentially be updated?

[bnoordhuis]

Not easily, I'm afraid, but the methods below are used in src/ - if their call sites don't pass a context as the first argument, they should be updated.

BooleanValue Call Delete Equals Evaluate Get GetEndColumn GetLineNumber GetPrivate GetStartColumn Has HasOwnProperty HasPrivate InstanceOf Instantiate InstantiateModule Int32Value New NewInstance NumberValue ReadHeader ReadValue Reject Resolve Run Set SetAccessor SetIntegrityLevel SetPrivate SetPrototype Then WriteValue

[Trott]

Update: I pulled together another ten or so tasks, and then @addaleax comes along and drops an automated way to find 148 good tasks. So: PROBLEM SOLVED! Thanks, Anna!

[addaleax]

In case anyone is wondering, it’s implementing a lint rule (https://github.com/addaleax/node/commit/2bdace784990012243ba1089363394401734c737) by either removing message arguments to assert functions if they are constant literals or making them contain more useful information (e.g. turning them into template literals)

[Trott]

Next issue: More attendees means we need more mentors! I think C+L works best when we have 1 mentor for every 5 attendees. Because it really does get just that busy. And we're nowhere near that.

So...time to ping some folks who haven't said they'd mentor but who I know are here:

@Fishrock123 (he just told me he'd likely be there, so I'll add him to the list, but pinging anyway) @trevnorris @lucamaraschi @fhinkel @cjihrig

Who else?

[cjihrig]

I have an early flight, and will be gone by the time of the C+L.

[addaleax]

I think @hashseed or some other googler was thinking about it?

[Trott]

@yosuke-furukawa

[refack]

If all else fails, I can IRC assist. We can open an IRC or slack channel.

[lance]

Count me in as well

[ofrobots]

I'll be there On Thu, Oct 5, 2017 at 4:05 PM Lance Ball notifications@github.com 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 https://github.com/nodejs/code-and-learn/issues/70#issuecomment-334615992, or mute the thread https://github.com/notifications/unsubscribe-auth/AAE0qY9wMVz-MaOZL78NVpa7zUOpSkiuks5spWDPgaJpZM4PSDU6 .

[boutell]

I'm working through some of this here:

https://github.com/nodejs/node/issues/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!

[boutell]

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

[hashseed]

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.

[richardlau]

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.

[gibfahn]

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.

[gibfahn]

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

[lance]

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

[lance]

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?

[gibfahn]

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.

[Trott]

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

[gibfahn]

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.

[refack]

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