test: improve test-fs-open-flags - #10908
Conversation
Sorry, something went wrong.
* use arrow funcion instead of function expression * add second argument to method assert.throws * check error messages from beginning to the end using ^ and $
edsadr
left a comment
There was a problem hiding this comment.
Minor changes but looks good =)
Sorry, something went wrong.
| ); | ||
|
|
||
| assert.throws( | ||
| () => stringToFlags(null), |
There was a problem hiding this comment.
same here
Sorry, something went wrong.
|
@edsadr, I only added new lines between lines 40 and 43. Should I update all the lines pointed out by you? |
Sorry, something went wrong.
I think my preference would be that you leave it as you currently have it, keeping the style of the existing file with the first argument on a line by itself. If you move all the args to the same line as the function call's opening parenthesis, then the linter will make you align all subsequent arguments, resulting in more churn. An then there will be yet more churn if that additional indentation causes a line to extend beyond 80 characters. So I think I actually would rather you keep it just like you already have it. That said, I don't feel strongly about it, so if @edsadr or anyone else has compelling reasons for wanting those changes, I'm OK with it. |
Sorry, something went wrong.
Trott
left a comment
There was a problem hiding this comment.
LGTM if CI is green
Sorry, something went wrong.
Sorry, something went wrong.
|
@Trott, can you give me a feedback about the CI? |
Sorry, something went wrong.
edsadr
left a comment
There was a problem hiding this comment.
@vinimdocarmo the CI is green, I was concerned about identation but as @Trott explains this could lead to more linting problems, then is fine and then.. LGTM 👍🏽
Sorry, something went wrong.
Sorry, something went wrong.
|
@vinimdocarmo PRs stay open for at least 48 hours and that goes up to 72 hours if it stretches into a weekend like this one will. (Fell free to leave a "bump" comment if it's still open on Monday.) |
Sorry, something went wrong.
* use arrow funcion instead of function expression * add second argument to method assert.throws * check error messages from beginning to the end using ^ and $ PR-URL: #10908 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Adrian Estrada <edsadr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
|
Landed in a024104, thanks for your contribution @vinimdocarmo |
Sorry, something went wrong.
* use arrow funcion instead of function expression * add second argument to method assert.throws * check error messages from beginning to the end using ^ and $ PR-URL: nodejs#10908 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Adrian Estrada <edsadr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
* use arrow funcion instead of function expression * add second argument to method assert.throws * check error messages from beginning to the end using ^ and $ PR-URL: nodejs#10908 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Adrian Estrada <edsadr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
* use arrow funcion instead of function expression * add second argument to method assert.throws * check error messages from beginning to the end using ^ and $ PR-URL: nodejs#10908 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Adrian Estrada <edsadr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
* use arrow funcion instead of function expression * add second argument to method assert.throws * check error messages from beginning to the end using ^ and $ PR-URL: nodejs#10908 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Adrian Estrada <edsadr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
* use arrow funcion instead of function expression * add second argument to method assert.throws * check error messages from beginning to the end using ^ and $ PR-URL: #10908 Backport-PR-URL: #13785 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Adrian Estrada <edsadr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
* use arrow funcion instead of function expression * add second argument to method assert.throws * check error messages from beginning to the end using ^ and $ PR-URL: #10908 Backport-PR-URL: #13785 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Adrian Estrada <edsadr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
* use arrow funcion instead of function expression * add second argument to method assert.throws * check error messages from beginning to the end using ^ and $ PR-URL: #10908 Backport-PR-URL: #13785 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Adrian Estrada <edsadr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Checklist
Affected core subsystem(s)
test