-
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
test-console: streamline arrow fn and refine inspect regex #11039
Conversation
removed unneccessary curly braces and return statement from inspect arrow function updated `assert.throws` regex to look for exact match at start of string
I personally find the removal of the curly braces more difficult to read, so I'm -1 on that part, but +1 on the other change. |
The way it is in this PR (no braces around an arrow function body if they are not required) is definitely the more common approach in our code base by a large margin: 474 vs 22. So I'm +1 on the change here (and making it 475 vs 21) in the hopes of one day getting us to a consistent style. (For the record, I don't have a strong personal preference for either style. I just want to stop seeing comments in both directions on pull requests. I want us to pick one, enforce it in the linter, and be done with it. :-D ) (EDIT: I do have a mild preference for explicit braces, but a much stronger preference for consistency.) |
I am not sure why the |
@jonniedarko don't worry about that, CI is 100% green. |
👍 |
removed unneccessary curly braces and return statement from inspect arrow function updated `assert.throws` regex to look for exact match at start of string PR-URL: #11039 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Landed in ca3d131 |
@jasnell landed with wrong subsystem in commit title? Maybe there is still time to fix it. |
@lpinca Nah, 10 min window has passed :( Edit: Just realized you commented well within the time-limit. Sorry :D |
@jasnell so I know for future, should it of been |
@jonniedarko You are correct! |
removed unneccessary curly braces and return statement from inspect arrow function updated `assert.throws` regex to look for exact match at start of string PR-URL: #11039 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
removed unneccessary curly braces and return statement from inspect arrow function updated `assert.throws` regex to look for exact match at start of string PR-URL: #11039 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
removed unneccessary curly braces and return statement from inspect arrow function updated `assert.throws` regex to look for exact match at start of string PR-URL: #11039 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
removed unnecessary curly braces and return statement from the
inspect
arrow functionupdated
assert.throws
regex to look for an exact match at the start of the stringChecklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes