Mentors for Code + Learn at Node Interactive 2017 about code-and-learn HOT 47 CLOSED

In case anyone is wondering, it’s implementing a lint rule (addaleax/node@2bdace7) 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)

from code-and-learn.

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.

from code-and-learn.

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.

from code-and-learn.

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.

from code-and-learn.

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.

from code-and-learn.

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

from code-and-learn.

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

from code-and-learn.

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

from code-and-learn.

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

from code-and-learn.

from code-and-learn.

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.

from code-and-learn.

I'll be there! 💪

from code-and-learn.

I will be there.

from code-and-learn.

I will be there

from code-and-learn.

I will be there.

from code-and-learn.

Me too

from code-and-learn.

I will be there

from code-and-learn.

I'll be there!

from code-and-learn.

I'm in!

from code-and-learn.

I'll be there as well

from code-and-learn.

where is it?

from code-and-learn.

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

from code-and-learn.

I'll be there.

from code-and-learn.

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

from code-and-learn.

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?

from code-and-learn.

I'm in.

from code-and-learn.

You know I am 💂

from code-and-learn.

I will be there as well

from code-and-learn.

I’ll be there

from code-and-learn.

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

from code-and-learn.

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

from code-and-learn.

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

from code-and-learn.

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!

from code-and-learn.

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?

from code-and-learn.

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

from code-and-learn.

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

from code-and-learn.

@yosuke-furukawa

from code-and-learn.

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

from code-and-learn.

Count me in as well

from code-and-learn.

from code-and-learn.

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!

from code-and-learn.

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.

from code-and-learn.

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.

from code-and-learn.

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

from code-and-learn.

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?

from code-and-learn.

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.

from code-and-learn.

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

from code-and-learn.