Skip to content

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
2 tasks
@jasnell
Copy link
Member

jasnell commented Aug 6, 2016

LGTM! Thank you!

README.md Outdated
Copy link
Member

@ChALkeR ChALkeR Aug 6, 2016

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

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

README.md Outdated
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

README.md Outdated
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

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
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
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
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
MylesBorins pushed a commit that referenced this pull request Sep 9, 2016
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
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
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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants