Skip to content

lint: wrap lines which include RegEx exceeding 80 chars#14607

Closed
thelostone-mc wants to merge 1 commit intonodejs:masterfrom
thelostone-mc:shitzu_develop
Closed

lint: wrap lines which include RegEx exceeding 80 chars#14607
thelostone-mc wants to merge 1 commit intonodejs:masterfrom
thelostone-mc:shitzu_develop

Conversation

@thelostone-mc
Copy link
Contributor

@thelostone-mc thelostone-mc commented Aug 3, 2017 •
edited by refack
Loading

Format commit wrapping lines containing RegEx and exceeding 80
chars.

Fixes: #14586

Files Changed:

  • test/addons-napi/test_properties/test.js
  • test/parallel/test-process-versions.js
  • test/parallel/test-repl.js
  • test/parallel/test-v8-serdes.js
  • test/parallel/test-whatwg-url-properties.js
  • test/parallel/test-zlib-not-string-or-buffer.js
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Aug 3, 2017
vsemozhetbyt
vsemozhetbyt reviewed Aug 3, 2017
View reviewed changes
test/parallel/test-repl.js Outdated
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Aug 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. There are some RegExps with the same pattern which are left unchanged (lines 193, 198, 203 etc).

  2. It seems the \b symbol after the ^ is unneeded.

Copy link
Contributor Author

@thelostone-mc thelostone-mc Aug 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vsemozhetbyt

  1. Well I tried that. On introducing a ^ in L193 which uses strict mode, I get the Octal literals are not allowed in strict mode error as below: (Same case as in other lines you had mentioned )
Unix data: "(function() { \"use strict\"; return 0755; })()\n 
^^^^\n\nSyntaxError: Octal literals are not allowed in strict mode.\n\nnode via Unix socket> node via Unix socket> ", expecting /^SyntaxError: Octal literals are not allowed in strict mode/
assert.js:42
  1. Yeah makes sense. Will do that! Can I go ahead and remove the\b even where I can't introduce a ^ cause of strict mode ?

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Aug 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Oh, sorry, I did not think this broke matching.

  2. I think that removing the \b is safe only if we place ^ before)

Copy link
Contributor Author

@thelostone-mc thelostone-mc Aug 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack and taken care of ^_^

refack
refack approved these changes Aug 3, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 on the wrapping!

thefourtheye
thefourtheye reviewed Aug 3, 2017
View reviewed changes
test/parallel/test-v8-serdes.js Outdated
Copy link
Contributor

@thefourtheye thefourtheye Aug 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Now the whole thing would fit in a single line.

Copy link
Contributor Author

@thelostone-mc thelostone-mc Aug 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thefourtheye True! But doesn't it look cleaner this way ?

Copy link
Contributor

@refack refack Aug 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm with @thefourtheye, also could drop the () => { } to get:

assert.throws(v8.Serializer, serializerTypeError);

Now that's clean ✨

Copy link
Contributor Author

@thelostone-mc thelostone-mc Aug 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well two against one !! Ack and taken care

Trott
Trott reviewed Aug 3, 2017
View reviewed changes
test/parallel/test-repl.js Outdated
Copy link
Member

@Trott Trott Aug 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit \b -> ^?

Copy link
Member

@Trott Trott Aug 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(And below two instances too?)

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Aug 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this somehow breaks matching: #14607 (comment)

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Aug 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some error messages quote erroneous code before the error message itself, so for these messages, we can't use ^ anchor (or we should add the /m flag beside).

Copy link
Member

@Trott Trott Aug 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, then \b it is!

Trott
Trott approved these changes Aug 3, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if CI is green.

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Aug 3, 2017

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

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Aug 3, 2017 •
edited
Loading

One aix fail in CI seems unrelated (parallel/test-async-wrap-getasyncid is crashed).

@Trott
Copy link
Member

Trott commented Aug 3, 2017

One aix fail in CI seems unrelated (parallel/test-async-wrap-getasyncid is crashed).

Yes, that's a known-flaky on that platform: #14599

@refack and possibly others are investigating.

CI is effectively green for this. 🎉

@refack refack self-assigned this Aug 3, 2017
@thelostone-mc
lint: wrap lines which include RegEx exceeding 80 chars
59d037f 
Format commit wrapping lines containing RegEx and exceeding 80
chars.

Fixes: nodejs#14586
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Aug 4, 2017 •
edited
Loading

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

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Aug 4, 2017

CI failures seem unrelated.

@refack refack mentioned this pull request Aug 6, 2017
Closed
refack pushed a commit to refack/node that referenced this pull request Aug 7, 2017
@thelostone-mc @refack
test: mitigate RegEx exceeding 80 chars
ad664ea 
Format commit wrapping lines containing RegEx and exceeding 80
chars.

PR-URL: nodejs#14607
Fixes: nodejs#14586
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@refack
Copy link
Contributor

refack commented Aug 7, 2017

Landed in ad664ea
Turned out nice if I may say so myself.

@refack refack closed this Aug 7, 2017
addaleax pushed a commit that referenced this pull request Aug 10, 2017
@thelostone-mc @addaleax
test: mitigate RegEx exceeding 80 chars
f73f659 
Format commit wrapping lines containing RegEx and exceeding 80
chars.

PR-URL: #14607
Fixes: #14586
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@addaleax addaleax mentioned this pull request Aug 13, 2017
Merged
@MylesBorins
Copy link
Contributor

MylesBorins commented Sep 19, 2017

This does not land cleanly in LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

@refack refack removed their assignment Oct 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott approved these changes

+3 more reviewers

@refack refack refack approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

lint: wrap long lines that include RegExp when possible

7 participants

@thelostone-mc @vsemozhetbyt @Trott @refack @MylesBorins @thefourtheye @nodejs-github-bot