FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test-console: streamline arrow fn and refine inspect regex by jonniedarko · Pull Request #11039 · nodejs/node · GitHub

/ node Public

test-console: streamline arrow fn and refine inspect regex - #11039

Closed
jonniedarko wants to merge 1 commit into
nodejs:masterfrom
jonniedarko:test-console-cleanup
ConversationCommits (1)ChecksFiles changed
Closed

test-console: streamline arrow fn and refine inspect regex#11039
jonniedarko wants to merge 1 commit into
nodejs:masterfrom
jonniedarko:test-console-cleanup

Conversation

jonniedarko commented Jan 27, 2017

Copy link
Copy Markdown
Contributor

removed unnecessary curly braces and return statement from the inspect arrow function

updated assert.throws regex to look for an exact match at the start of the string

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

test-console: streamline arrow fn and refine regex
8c5aa21 
removed unneccessary curly braces and return statement from inspect arrow function

updated `assert.throws` regex to look for exact match at start of string
nodejs-github-bot added test Issues and PRs related to the tests. lts-watch-v6.x labels Jan 27, 2017

Trott commented Jan 27, 2017

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/6083/

mscdex added the console Issues and PRs related to the console subsystem. label Jan 27, 2017

evanlucas commented Jan 28, 2017

Copy link
Copy Markdown
Contributor

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.

Trott commented Jan 28, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

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.)

jonniedarko commented Jan 30, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I am not sure why the test/arm CI is failing. when I follow the details link it says successful. Is there something I am missing

lpinca commented Jan 30, 2017

Copy link
Copy Markdown
Member

@jonniedarko don't worry about that, CI is 100% green.

jonniedarko commented Jan 30, 2017

Copy link
Copy Markdown
Contributor Author

👍

outsideris mentioned this pull request Jan 30, 2017
Closed
jasnell pushed a commit that referenced this pull request Jan 30, 2017
test-console: streamline arrow fn and refine regex
ca3d131 
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>

jasnell commented Jan 30, 2017

Copy link
Copy Markdown
Member

Landed in ca3d131

jasnell closed this Jan 30, 2017

lpinca commented Jan 30, 2017

Copy link
Copy Markdown
Member

@jasnell landed with wrong subsystem in commit title? Maybe there is still time to fix it.

thefourtheye commented Jan 30, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

@lpinca Nah, 10 min window has passed :(

Edit: Just realized you commented well within the time-limit. Sorry :D

jonniedarko deleted the test-console-cleanup branch January 30, 2017 19:50

jonniedarko commented Jan 30, 2017

Copy link
Copy Markdown
Contributor Author

@jasnell so I know for future, should it of been test rather than test-console?

thefourtheye commented Jan 30, 2017

Copy link
Copy Markdown
Contributor

@jonniedarko You are correct!

evanlucas pushed a commit that referenced this pull request Jan 31, 2017
test-console: streamline arrow fn and refine regex
c6af766 
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>
italoacasas mentioned this pull request Jan 31, 2017
Merged
jasnell pushed a commit that referenced this pull request Mar 7, 2017
test-console: streamline arrow fn and refine regex
ea06647 
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>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
test-console: streamline arrow fn and refine regex
3e9ce77 
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>
MylesBorins mentioned this pull request Mar 9, 2017
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
Trott Trott approved these changes
lpinca lpinca approved these changes
+1 more reviewer
gibfahn gibfahn approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

console Issues and PRs related to the console subsystem. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL