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

Jump to bottom

meta: clarify collaborators & ctc members relationships #7996

Closed
wants to merge 1 commit into from
Closed

meta: clarify collaborators & ctc members relationships #7996

wants to merge 1 commit into from

Conversation

yorkie
Copy link
Contributor

@yorkie yorkie commented Aug 6, 2016

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

meta

Description of change

Clarifying collaborators & ctc members relationships inspired by this conversation
#7183 (comment)

This patches only the README, so I removed test relevant checks :-)

R= @jasnell

@yorkie yorkie added the meta Issues and PRs related to the general management of the project. label Aug 6, 2016
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Aug 6, 2016
@yorkie yorkie mentioned this pull request Aug 6, 2016
Closed
2 tasks
@jasnell
Copy link
Member

jasnell commented Aug 6, 2016

LGTM! Thank you!

ChALkeR
ChALkeR reviewed Aug 6, 2016
View reviewed changes
README.md
All of the CTC members are also Collaborators, we use Collaborators usually
to include CTC members as well.

Therefore Collaborators(include CTC members) would follow the
Copy link
Member

@ChALkeR ChALkeR Aug 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Something is wrong on this line. Perhaps, Collaborators (including CTC members)? Also, why is would needed here? Also not sure about Therefore.

@yorkie
Copy link
Contributor Author

yorkie commented Aug 6, 2016 •
edited
Loading

Fixed nits by @ChALkeR and still leave the word Therefore, thank you :-)

Trott
Trott reviewed Aug 6, 2016
View reviewed changes
README.md
@@ -319,8 +319,12 @@ more information about the governance of the Node.js project, see
* [zkat](https://github.com/zkat) -
**Kat Marchán** <kzm@sykosomatic.org>

Collaborators & CTC members follow the [COLLABORATOR_GUIDE.md](./COLLABORATOR_GUIDE.md) in
maintaining the Node.js project.
All of the CTC members are also Collaborators, we use Collaborators usually
Copy link
Member

Choose a reason for hiding this comment

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

There's a comma splice in there and a personal pronoun that should be removed. I'd simplify the whole sentence to just this:

Note that all CTC members are also Collaboartors.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I'd suggest removing all the added text and just changing the original sentence from starting with Collaborators & CTC members follow... to Collaborators (which includes CTC members) follow...

@yorkie
Copy link
Contributor Author

yorkie commented Aug 6, 2016

Fixed again @Trott

ChALkeR
ChALkeR reviewed Aug 6, 2016
View reviewed changes
README.md
@@ -319,8 +319,9 @@ more information about the governance of the Node.js project, see
* [zkat](https://github.com/zkat) -
**Kat Marchán** <kzm@sykosomatic.org>

Collaborators & CTC members follow the [COLLABORATOR_GUIDE.md](./COLLABORATOR_GUIDE.md) in
maintaining the Node.js project.
Collaborators & CTC members (which includes CTC members) follow the
Copy link
Member

Choose a reason for hiding this comment

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

& CTC members is probably not needed here.

@yorkie
Copy link
Contributor Author

yorkie commented Aug 7, 2016

Fixed @ChALkeR :-)

@Trott
Copy link
Member

Trott commented Aug 7, 2016

LGTM

@yorkie
Copy link
Contributor Author

yorkie commented Aug 7, 2016

Rebased commits to d743a8b, I will land it after 48 hours without running CI because this only patches README which is not covered by any tests, if CI is still required to run, tell me please :-)

BTW, ping @ChALkeR is this pr looks good to you?

@ChALkeR
Copy link
Member

ChALkeR commented Aug 7, 2016

@yorkie Yes, it does LGTM. Note that I'm not a native English speaker, though, so my LGTM is less valuable here.

because this only patches README which is not covered by any tests

It's not yet, but soon will be =). Not in under 48 hours ofc.
This passes the remark-lint tests, I manually verified.

@yorkie
Copy link
Contributor Author

yorkie commented Aug 7, 2016

It's not yet, but soon will be =). Not in under 48 hours ofc.
This passes the remark-lint tests, I manually verified.

So may I land it today?

@evanlucas
Copy link
Contributor

LGTM

@ChALkeR
Copy link
Member

ChALkeR commented Aug 8, 2016

@yorkie Yes =).

@cjihrig
Copy link
Contributor

cjihrig commented Aug 8, 2016

LGTM

@yorkie
doc: clarify collaborators & ctc members relationships
4f24f58 
inspired by this conversation
nodejs#7183 (comment)

PR-URL: nodejs#7996
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@yorkie
Copy link
Contributor Author

yorkie commented Aug 8, 2016

Landed at accaa34 :-)

@yorkie yorkie closed this Aug 8, 2016
yorkie added a commit that referenced this pull request Aug 8, 2016
@yorkie
doc: clarify collaborators & ctc members relationships
accaa34 
inspired by this conversation
#7183 (comment)

PR-URL: #7996
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@yorkie yorkie deleted the improve/readme branch August 8, 2016 15:25
@cjihrig cjihrig mentioned this pull request Aug 8, 2016
Closed
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
@yorkie @cjihrig
doc: clarify collaborators & ctc members relationships
7d0e5a0 
inspired by this conversation
#7183 (comment)

PR-URL: #7996
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Sep 9, 2016
@yorkie
doc: clarify collaborators & ctc members relationships
65abbfe 
inspired by this conversation
#7183 (comment)

PR-URL: #7996
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 28, 2016
@yorkie
doc: clarify collaborators & ctc members relationships
cce91d7 
inspired by this conversation
#7183 (comment)

PR-URL: #7996
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
@yorkie @rvagg
doc: clarify collaborators & ctc members relationships
5c33194 
inspired by this conversation
#7183 (comment)

PR-URL: #7996
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
@yorkie @MylesBorins
doc: clarify collaborators & ctc members relationships
e74daad 
inspired by this conversation
#7183 (comment)

PR-URL: #7996
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 26, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@yorkie @jasnell @Trott @ChALkeR @evanlucas @cjihrig @MylesBorins @nodejs-github-bot