-
Notifications
You must be signed in to change notification settings - Fork 28.8k
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
cli: fix the misalinged text on "node --help" #10948
Conversation
6e97b90
to
cd89840
Compare
Thanks @aashil! Can you prefix the commit message with |
Does the |
LGTM - thanks! |
Agree with @cjihrig's concern. If this causes any lines to go over 80, then that should be fixed. Otherwise LGTM |
@evanlucas Sure, will do it. |
cd89840
to
1cf525b
Compare
Because you didn't write it yet! :-D |
@cjihrig Can I help with setting it up? If yes, I would like to submit an issue and start working on it. |
I would just add a test to this PR. You can spawn |
I think we already have cpplint setup which tests for the 80 character limit here. From what I understand, the CI should automatically boot up the cpplint on submitting a PR. Please correct me if I am wrong. |
Linting would run on the source code. I was referring to the actual message printed to the console. |
Ah, makes sense. Can you please point me to the file where I need to write the test ? Thank you. |
I would create a new test in |
Thanks. Will add the test soon. |
1cf525b
to
37860ef
Compare
@cjihrig I wrote a test as discussed but the test is reading the message from the Error:
|
@aashil |
@Trott Added the space after it. Still the same error. This time I did |
37860ef
to
314bc85
Compare
#if HAVE_OPENSSL | ||
" --tls-cipher-list=val use an alternative default TLS cipher " | ||
" --tls-cipher-list=val use an alternative default TLS cipher " | ||
"list\n" | ||
" --use-bundled-ca use bundled CA store" |
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 and use-openssl-ca
need to be aligned.
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.
Fixed.
ab9cebc
to
ed2faef
Compare
It appears that
|
The ARM thing is just a problem with the GitHub widget. It's just the FIPS issue that we need to figure out. As best as I can tell from looking at So given that, it seems like the options are:
Looking at I'm not sure I'm communicating the idea clearly above, but that may be OK for now because I'm starting to wonder if trying to have the test be in this PR may be creating more problems than it's solving. Maybe we should strip it out of this and open a separate PR for it. Then the alignment fix for the usage text can land sooner and the test can take as much time as it takes. Normally, I'm not in favor of landing tests after functionality, but this isn't really functionality. It's formatting. I'm OK with the test coming later. We've gone this long without a test for it... In case I'm missing an obvious solution that can easily be implemented, thus invalidating nearly everything I've written above: /cc @nodejs/testing |
@aashil Is it OK with you if I land the alignment fix and we push the test issue to another PR? |
Sure, Rich. I would like to work on the test issue PR as well. Will create a new PR in the weekend. |
The alignment of the argument descriptions in the "node --help" text is off. This commit fixes the issue by adding two spaces before each of the argument description. PR-URL: nodejs#10948 Fixes: nodejs#10935 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Josh Gavant <josh.gavant@outlook.com>
ed2faef
to
5d27cc1
Compare
Updated to just include changes to alignment in help message. |
Landed in 5d27cc1. |
This isn't landing cleanly on v7.x. Mind opening a backport pr targetting v7.x-staging? |
@evanlucas Might be best to backport 6ff3b03 first. |
I can backport both if you want. |
@aashil that would be awesome! Let us know if you have any questions on how to backport! Thanks! |
From what I understand about backporting, first I need to create a branch off upstream/v7.x-staging and install the node version (7.x) in that branch. Thereafter, I need to make a copy of those commits (cherry-pick) and make sure it runs of v7.x. Just want to make sure I am on the right path. |
The alignment of the argument descriptions in the "node --help" text is off. This commit fixes the issue by adding two spaces before each of the argument description. PR-URL: nodejs#10948 Fixes: nodejs#10935 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Josh Gavant <josh.gavant@outlook.com>
The alignment of the argument descriptions in the "node --help" text is off. This commit fixes the issue by adding two spaces before each of the argument description. PR-URL: nodejs#10948 Fixes: nodejs#10935 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Josh Gavant <josh.gavant@outlook.com>
The alignment of the argument descriptions in the "node --help"
text is off. This commit fixes the issue by adding two spaces
before each of the argument description.
Fixes: #10935
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
cli