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.

Sign up for GitHub

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

Update README #34

Merged
merged 1 commit into from
Nov 1, 2017
Merged

Update README #34

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
[![npm](https://img.shields.io/npm/v/node-core-utils.svg?style=flat-square)](https://npmjs.org/package/node-core-utils)
[![Build Status](https://travis-ci.org/joyeecheung/node-core-utils.svg?branch=master)](https://travis-ci.org/joyeecheung/node-core-utils)
[![codecov](https://codecov.io/gh/joyeecheung/node-core-utils/branch/master/graph/badge.svg)](https://codecov.io/gh/joyeecheung/node-core-utils)
[![Known Vulnerabilities](https://snyk.io/test/github/joyeecheung/node-core-utils/badge.svg)](https://snyk.io/test/github/joyeecheung/node-core-utils)

CLI tools for Node.js Core collaborators

Expand All @@ -10,7 +11,9 @@ CLI tools for Node.js Core collaborators
First, [follow these instructions](https://help.github.com/articles/creating-a-personal-access-token-for-the-command-line/)
to create a personal access token.

Note: You don't need to check any boxes, these tools only require public access(for now).
Note: We need to read the email of the PR author in order to check if it matches
the email of the commit author. This requires checking the box `user:email` when
you create the personal access token (you can edit the permission later as well).

Then create a file named `.ncurc` under your `$HOME` directory (`~/.ncurc`);

Expand Down Expand Up @@ -44,8 +47,7 @@ This one is inspired by Evan Lucas's [node-review](https://github.com/evanlucas/
- [x] Generate `Fixes`
- [x] Generate `Refs`
- [x] Check for CI runs
- [ ] Check if commiters match authors
- Only when `"authorAssociation": "FIRST_TIME_CONTRIBUTOR"`
- [x] Check if commiters match authors
- [x] Check 48-hour wait
- [x] Check two TSC approval for semver-major
- [ ] Warn new commits after reviews
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm..also I am not quite sure how this one should be handled because in practice, we still count reviews that give approval before new commits, as long as they don't object afterwards.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, there was a conversation at some point that people who comment "LGTM" want that to be dismissed if a new commit comes in.
nodejs/node#13413 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm..I think I can not get a conclusion from that thread, or did I miss something? Should we open an issue in the core repo to get this settled?

Expand Down