lint: wrap lines which include RegEx exceeding 80 chars by thelostone-mc · Pull Request #14607 · nodejs/node · GitHub

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

There are some RegExps with the same pattern which are left unchanged (lines 193, 198, 203 etc).
It seems the \b symbol after the ^ is unneeded.

@vsemozhetbyt

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

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 ?

Oh, sorry, I did not think this broke matching.
I think that removing the \b is safe only if we place ^ before)

Ack and taken care of ^_^

💯 on the wrapping!

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

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

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

assert.throws(v8.Serializer, serializerTypeError);

Now that's clean ✨

Well two against one !! Ack and taken care

Nit \b -> ^?

(And below two instances too?)

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

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

OK, then \b it is!

LGTM if CI is green.

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

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

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

Format commit wrapping lines containing RegEx and exceeding 80 chars. Fixes: nodejs#14586

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

CI failures seem unrelated.

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>

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

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>

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

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 reviewed Aug 3, 2017

View reviewed changes

refack approved these changes Aug 3, 2017

View reviewed changes

thefourtheye reviewed Aug 3, 2017

View reviewed changes

thelostone-mc force-pushed the shitzu_develop branch from 5279834 to 91b76ad Compare August 3, 2017 19:46

Trott reviewed Aug 3, 2017

View reviewed changes

Trott approved these changes Aug 3, 2017

View reviewed changes

vsemozhetbyt approved these changes Aug 3, 2017

View reviewed changes

thefourtheye approved these changes Aug 3, 2017

View reviewed changes

refack self-assigned this Aug 3, 2017

lint: wrap lines which include RegEx exceeding 80 chars

59d037f

Format commit wrapping lines containing RegEx and exceeding 80 chars. Fixes: nodejs#14586

thelostone-mc force-pushed the shitzu_develop branch from 91b76ad to 59d037f Compare August 4, 2017 06:03

refack mentioned this pull request Aug 6, 2017

CR - max-len: {ignoreRegExpLiterals: true} is too permissive eslint/eslint#9059

Closed

refack closed this Aug 7, 2017

addaleax mentioned this pull request Aug 13, 2017

v8.4.0 proposal #14811

Merged

MylesBorins added the dont-land-on-v6.x label Sep 19, 2017

refack removed their assignment Oct 20, 2018

lint: wrap lines which include RegEx exceeding 80 chars by thelostone-mc · Pull Request #14607 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

lint: wrap lines which include RegEx exceeding 80 chars#14607thelostone-mc wants to merge 1 commit intonodejs:masternodejs/node:masterfrom thelostone-mc:shitzu_developthelostone-mc/node:shitzu_developCopy head branch name to clipboard

Conversation

thelostone-mc commented Aug 3, 2017 • edited by refack Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Aug 3, 2017

Uh oh!

vsemozhetbyt commented Aug 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Aug 3, 2017

Uh oh!

vsemozhetbyt commented Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsemozhetbyt commented Aug 4, 2017

Uh oh!

refack commented Aug 7, 2017

Uh oh!

MylesBorins commented Sep 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

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

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

vsemozhetbyt commented Aug 3, 2017 •
edited
Loading

vsemozhetbyt commented Aug 4, 2017 •
edited
Loading