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.

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

[Doc] Make the background section concise and improve its formality #18928

Closed
wants to merge 1 commit into from

Conversation

wla80
Copy link

@wla80 wla80 commented Feb 22, 2018

Improve the writing quality of the background section in maintaining-V8.md

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Feb 22, 2018
Copy link
Contributor

@mmarchini mmarchini left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I left a couple of comments, but other than that LGTM

for maintaining the V8 branches in Node.js LTS and Current releases and how the
Node.js and V8 teams at Google can help.
different compared to the support horizon for Node.js. As a result, Node.js
needs to support a version of V8 longer than what upstream needs to support.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can change a version with versions or multiple versions since we support one version of V8 for each release line.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for pointing out. I will revise it:)

LTS supported branch.

This document attempts to outline the current maintaining processes, proposes
a workflow for maintaining the V8 branches in both Node.js LTS and current releases, and
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you be so kind and wrap this paragraph at 80 characters?

Copy link
Author

Choose a reason for hiding this comment

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

Could you specify how can I achieve this without losing information. Also, I think this paragraph can be removed since the sections discussed in the paragraph were already explicitly stated in the document.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I meant wrap every line in the paragraph at 80 characters. Otherwise, our linter will complain :)

This document attempts to outline the current maintaining processes, proposes
a workflow for maintaining the V8 branches in both Node.js LTS and current
releases, and discusses how the Node.js and V8 teams at Google can help.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I will fix this. Sorry for the confusion and thank you again.

Copy link
Contributor

@claudiorodriguez claudiorodriguez left a comment

Choose a reason for hiding this comment

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

LGTM, just a tiny nit: commit message doesn't fit the guidelines

to support. V8 branches in Node.js lack of official maintaining process due
to a missing LTS supported branch.

This document attempts to outline the current maintaining processes, proposes
Copy link
Member

Choose a reason for hiding this comment

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

Nit: maintenance processes seems more natural to me than maintaining processes

Copy link
Author

Choose a reason for hiding this comment

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

Agreed and fixed. thanks:)

@wla80
Copy link
Author

wla80 commented Feb 23, 2018

Hello @claudiorodriguez, I just uploaded a new commit to address the incorrect commit message format. Please let me know if I am still doing it wrong. thanks

@mmarchini mmarchini added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 27, 2018
@mmarchini
Copy link
Contributor

@mmarchini
Copy link
Contributor

mmarchini commented Feb 27, 2018

Landed in a27e6d7, thanks! 😄

@mmarchini mmarchini closed this Feb 27, 2018
mmarchini pushed a commit that referenced this pull request Feb 27, 2018
PR-URL: #18928
Reviewed-By: Claudio Rodriguez <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this pull request Mar 5, 2018
PR-URL: nodejs#18928
Reviewed-By: Claudio Rodriguez <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 6, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#18928
Reviewed-By: Claudio Rodriguez <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit to jasnell/node that referenced this pull request Aug 17, 2018
PR-URL: nodejs#18928
Reviewed-By: Claudio Rodriguez <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 6, 2018
Backport-PR-URL: #22380
PR-URL: #18928
Reviewed-By: Claudio Rodriguez <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants