-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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.
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
docs: added a License #36278
docs: added a License #36278
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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. |
Can you move it above the definitions to fix the linter issue please? |
Sure. |
I think this is almost ready, there is just one point left:
|
## License | ||
|
||
Node.js is available under the | ||
[MIT license](https://opensource.org/licenses/MIT). Node.js also includes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should link to the LICENSE
file instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The license file is linked just below, are you suggesting to also remove the See LICENSE for the full license text
sentence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @jasnell I think this is the last thing we need to clarify before this can land.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, removing the last sentence and moving the link to the LICENSE
file should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasnell sorry I missed your comment before landing, do you want me to revert?
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 |
Thank you. I will remember it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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>
Landed in 91fe3de |
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>
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>
Added a License in README.md file.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes