-
Notifications
You must be signed in to change notification settings - Fork 30k
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: minor doc improvements #17722
doc: minor doc improvements #17722
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.
In general, LGTM, although I'm a little concerned about the amount of git churn here. People obviously don't bisect the docs as much as actual code, but still useful to have a good history to refer back to.
1. Run `ninja -C out/Release` to produce a compiled release binary. | ||
1. Lastly, make symlink to `./node` using `ln -fs out/Release/node node`. | ||
|
||
When running `ninja -C out/Release` you will see output similar to the following |
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.
While we're here, can you add a comma after ninja -C out/Release
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.
I would rather keep this PR only about stylistic changes.
I rebase tomorrow |
2. Make sure that the local staging branch is up to date with the remote | ||
3. Create a new branch off of the staging branch | ||
1. Make sure that the local staging branch is up to date with the remote | ||
1. Create a new branch off of the staging branch |
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.
I prefer the numbering of the steps before this change (i.e. When it didn't reset to 1 after the examples).
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.
Ok, I checked and most files use 1. 2. 3...
even though there is at least one file that uses 1. 1. 1...
. I personally prefer the latter as it allows to reorder things easily / removing a single entry without creating any churn.
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.
My point is that this change has renumbered the steps so that instead of steps 1-10 it now reads steps 1-3, 1, 1-6.
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.
You are right. I did not check that anymore. I just removed the commit as our style currently does not really rely on it. I guess it might be introduced as a general thing or it should not be used further at all.
d29f5f3
to
a009267
Compare
Rebased and I removed a commit about changing the numbers. |
Commit message nit: |
This will also use a proper indentation as a couple of entries had a extra indentation of two spaces.
a009267
to
eef95c3
Compare
Rebased |
This will also use a proper indentation as a couple of entries had a extra indentation of two spaces. PR-URL: nodejs#17722 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
PR-URL: nodejs#17722 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
PR-URL: nodejs#17722 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
This will also use a proper indentation as a couple of entries had a extra indentation of two spaces. PR-URL: #17722 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
PR-URL: #17722 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
PR-URL: #17722 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
This will also use a proper indentation as a couple of entries had a extra indentation of two spaces. PR-URL: #17722 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
PR-URL: #17722 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
PR-URL: #17722 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
This will also use a proper indentation as a couple of entries had a extra indentation of two spaces. PR-URL: #17722 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
PR-URL: #17722 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
PR-URL: #17722 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
PR-URL: #17722 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
PR-URL: #17722 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
Landed the first commit on 6.x and 8.x (the others conflicted). |
PR-URL: #17722 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
PR-URL: #17722 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
PR-URL: #17722 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
PR-URL: #17722 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
Seems like some parts of the docs are currently actually not linted? Otherwise these files would probably result in a couple of errors. I went ahead and fixed things as I think is best. This does not mean they will already work with our lint rules and they might need further work but I think it is a good step into the right direction.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
doc