lint: wrap long lines that include RegExp when possible

There are several places in the codebase where lines are longer than 80 characters but could be wrapped.

Possible solutions:

Simply wrap the line before the RegExp
Define the RegExp as a const before it's use

Mitigations:

It some case the RegExp alone is > 80 chars, but the line could still be wrapped so it won't be longer than neccecery.

Calling dibs if this hasn't been taken yet ^_^

@refack

@refack Would this be good way to go about it ? Or did you have a better solution in mind for
https://github.com/nodejs/node/blob/master/test/parallel/test-whatwg-url-properties.js#L47

/TypeError: Cannot set property origin of \[object URL\] which has only a getter$/

to

new RegExp([
   'TypeError:',
   'Cannot set property origin of \\[object URL\\] which has only a getter$'
].join(' '))

to

new RegExp([
   'TypeError:',
   'Cannot set property origin of \\[object URL\\] which has only a getter$'
].join(' '))

The main motivation is the keep thing as readable as possible.
So... The general idea is not to use new RegExp() unless there are actual variables involved.
But the case you pointed out could be "mitigated" by wrapping the other parameters
from:

assert.throws(() => url.origin = 'http://foo.bar.com:22',
              /TypeError: Cannot set property origin of \[object URL\] which has only a getter$/);

assert.throws(
  () => url.origin = 'http://foo.bar.com:22',
  /TypeError: Cannot set property origin of \[object URL\] which has only a getter$/
);

We do end up with a line that 85 chars long, but it's still better than before.

But in this specific case since assert.throws also accepts a string as is second argument, and does a simple string compare, which is semantically the same as an anchored RegExp (/^xxx$/), you could compose a string literal

@refack

@refack We do not advise strings as error arguments here: https://nodejs.org/api/assert.html#assert_assert_throws_block_error_message (see notes in the chapter end).

But in this specific case since assert.throws also accepts a string as is second argument, and does a simple string compare,

That is wrong (although, to be clear, the API is at fault, not you). A string as the second argument will be the message printed if the code does not throw. Avoid it entirely because everyone makes this mistake. It is an unfortunate legacy issue in the API.

@refack

IMO https://github.com/nodejs/node/blob/master/test/parallel/test-whatwg-url-properties.js#L47 could be left as is and removed from the list. If you want to marginally improve it, then the indentation modification option suggested by @refack is the way to go:

assert.throws(
  () => url.origin = 'http://foo.bar.com:22',
  /TypeError: Cannot set property origin of \[object URL\] which has only a getter$/
);

You could also consider replacing the regexp with a function, but going that route solely to avoid some line-wrapping seems misguided to me.

(mixed up assert.throws and common.expectsError. P.S. expectsError is not suited here since it's meant to assert Errors that have a code field.)

IMHO wrapping will be the best, and also if we're touching this I believe the RegExp should be anchored for start of line (/^xxx) as well (if the test still passes).

@refack

@refack
Doesn't make these into constants as it's a custom message & further wrapping isn't possible :

Scnearios where the string can be made into a constant but that would make matters worse as defining the constant would hog more characters than the current state :

https://github.com/nodejs/node/blob/master/test/parallel/test-v8-serdes.js#L143

And when the text is already a constant and exceeds the limit, can't do anything about it, can we?

Yes I'm aware this looks like a long list of complaints but I'm just voicing out my thoughts.
and yup anchoring the start of the RegExp with ^ is definitely happening ^_^

@adityaanandmc for the first two and last two, IMHO simply wrapping will look nicer:

const expected = /^TypeError: "buffer" argument must be a string, Buffer, TypedArray, or DataView$/;
...
    {
      client: client_unix,
      send: '(function(a, a, b) { "use strict"; return a + b + c; })()',
      expect: /\bSyntaxError: Duplicate parameter name not allowed in this context/
    },

to:

const expected =
  /^TypeError: "buffer" argument must be a string, Buffer, TypedArray, or DataView$/;
...
    {
      client: client_unix,
      send: '(function(a, a, b) { "use strict"; return a + b + c; })()',
      expect:
       /\bSyntaxError: Duplicate parameter name not allowed in this context/
    },

Same trick can be done for https://github.com/nodejs/node/blob/master/test/parallel/test-v8-serdes.js#L143 even just before the block scope to save two indentation chars (but add a comment why it's there).

This issue turns out to be an exercise in creative typography 😉

test: mitigate RegEx exceeding 80 chars

Navigation Menu

Repository navigation

lint: wrap long lines that include RegExp when possible #14586

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

lint: wrap long lines that include RegExp when possible #14586

Description

Activity

thelostone-mc commented on Aug 2, 2017

thelostone-mc commented on Aug 2, 2017

refack commented on Aug 2, 2017

vsemozhetbyt commented on Aug 2, 2017

Trott commented on Aug 2, 2017

Trott commented on Aug 2, 2017

refack commented on Aug 2, 2017

thelostone-mc commented on Aug 3, 2017

refack commented on Aug 3, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions