FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

docs: added a License by iam-frankqiu · Pull Request #36278 · nodejs/node · GitHub

/ node Public

docs: added a License - #36278

Merged
aduh95 merged 1 commit into
nodejs:masterfrom
iam-frankqiu:docs_license
Dec 15, 2020
ConversationCommits (1)ChecksFiles changed
Merged

docs: added a License#36278
aduh95 merged 1 commit into
nodejs:masterfrom
iam-frankqiu:docs_license

Conversation

iam-frankqiu commented Nov 26, 2020

Copy link
Copy Markdown
Contributor

Added a License in README.md file.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Nov 26, 2020
aduh95 requested changes Nov 26, 2020
View reviewed changes

aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Our license file is https://github.com/nodejs/node/blob/master/LICENSE. I don't think there is any value to have that information in the README.

Trott reviewed Nov 28, 2020
View reviewed changes
Comment thread README.md Outdated
iam-frankqiu requested a review from Trott November 29, 2020 12:34
Trott requested changes Nov 29, 2020
View reviewed changes

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

"License" would need to be added to the table of contents around line 24 if we do it this way.

Can we maybe move this further down in the document? Maybe after "Contributing to Node.js" and before "Current Project Team Members"? Or maybe even all the way at the bottom of the document?

Trott commented Nov 29, 2020

Copy link
Copy Markdown
Member

"License" would need to be added to the table of contents around line 24 if we do it this way.

Can we maybe move this further down in the document? Maybe after "Contributing to Node.js" and before "Current Project Team Members"? Or maybe even all the way at the bottom of the document?

By the way, if you prefer that I add a fixup commit to make these changes rather than implementing them yourself, let me know. I know it can be tedious when a small change like this gets a bunch of requests for changes.

aduh95 commented Nov 29, 2020

Copy link
Copy Markdown
Contributor

Can you move it above the definitions to fix the linter issue please?

iam-frankqiu commented Nov 30, 2020

Copy link
Copy Markdown
Contributor Author

Can you move it above the definitions to fix the linter issue please?

Sure.

iam-frankqiu requested a review from Trott November 30, 2020 06:17

aduh95 commented Nov 30, 2020

Copy link
Copy Markdown
Contributor

I think this is almost ready, there is just one point left:

"License" would need to be added to the table of contents around line 24 if we do it this way.

iam-frankqiu commented Nov 30, 2020

Copy link
Copy Markdown
Contributor Author

"License" would need to be added to the table of contents around line 24 if we do it this way

I think this is almost ready, there is just one point left:

"License" would need to be added to the table of contents around line 24 if we do it this way.

Ok. It's done finally. Thanks to @aduh95 @Trott.

jasnell reviewed Nov 30, 2020
View reviewed changes
Comment thread README.md Outdated

jasnell Nov 30, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This should link to the LICENSE file instead.

aduh95 Nov 30, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The license file is linked just below, are you suggesting to also remove the See LICENSE for the full license text sentence?

mhdawson Dec 9, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ping @jasnell I think this is the last thing we need to clarify before this can land.

jasnell Dec 15, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes, removing the last sentence and moving the link to the LICENSE file should work.

aduh95 Dec 15, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@jasnell sorry I missed your comment before landing, do you want me to revert?

iam-frankqiu requested a review from jasnell December 2, 2020 02:45

Trott commented Dec 2, 2020

Copy link
Copy Markdown
Member

No need to fix it in the PR but here's a commit message nit-pick for whoever lands this: The first word after the subsystem should be an imperative mood verb. So add instead of added.

iam-frankqiu commented Dec 3, 2020

Copy link
Copy Markdown
Contributor Author

No need to fix it in the PR but here's a commit message nit-pick for whoever lands this: The first word after the subsystem should be an imperative mood verb. So add instead of added.

Thank you. I will remember it.

aduh95 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 3, 2020
mhdawson approved these changes Dec 9, 2020
View reviewed changes

mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

doc: add license info to the README
91fe3de 
PR-URL: nodejs#36278
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>

aduh95 commented Dec 15, 2020

Copy link
Copy Markdown
Contributor

Landed in 91fe3de

aduh95 merged commit 91fe3de into nodejs:master Dec 15, 2020
targos pushed a commit that referenced this pull request Dec 21, 2020
doc: add license info to the README
7a34452 
PR-URL: #36278
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
targos mentioned this pull request Dec 22, 2020
Merged
targos pushed a commit that referenced this pull request May 1, 2021
doc: add license info to the README
647ec70 
PR-URL: #36278
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
danielleadams mentioned this pull request May 3, 2021
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

aduh95 aduh95 approved these changes
Trott Trott approved these changes
jasnell Awaiting requested review from jasnell
+1 more reviewer
mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. doc Issues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL