Skip to content

Conversation

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Jun 7, 2017

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

dns, repl

If I get this correctly, a RegExp part with a quantifier '0 or more' is redundant if:

  • it is used at the very beginning or the very end of the RegExp;
  • it is used in a boolean context (test());
  • or it is not captured in match()/exec() results (full or partial) or does not affect match indices.

However, these parts may improve readability and comprehensibility. If this is the case for the changed fragments, feel free to reject this PR.

Also, I can miss some other nuances, so, please, correct me.

@nodejs-github-bot nodejs-github-bot added dns Issues and PRs related to the dns subsystem. repl Issues and PRs related to the REPL subsystem. labels Jun 7, 2017
@vsemozhetbyt
Copy link
Contributor Author

lib/repl.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Also may as well change this to a regex.test() since the return value is discarded.

lib/repl.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

just depth would be fine instead of depth: depth

Copy link
Contributor Author

@vsemozhetbyt vsemozhetbyt Jun 7, 2017

Choose a reason for hiding this comment

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

This seems to be the object of another PR. But I recall that some collaborators oppose partial shorthands: this is preferred to be consistent, all shorthands or all full pairs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't worry about this for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

@vsemozhetbyt Didn't know about the guideline :p I had a review in my PR for shorthand that's why!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not a guideline yet, just a comment in some PR or proposition that I recall)

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Jun 7, 2017

The first CI was green.

I've replaced match() with test() in a boolean context as was proposed.

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

1 flaky test fails on arm, seems unrelated.

lib/repl.js Outdated
Copy link
Member

@Trott Trott Jun 7, 2017

Choose a reason for hiding this comment

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

Micro-nit: Might be better as /\bfunction\b/ or /\sfunction\s/?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done)

@vsemozhetbyt
Copy link
Contributor Author

@vsemozhetbyt
Copy link
Contributor Author

Sorry, it seems I have a bit wider PR emerging, so I will close this for now, as it will interweave with the new one.

@vsemozhetbyt vsemozhetbyt deleted the redundant-regexp branch June 7, 2017 22:23
@vsemozhetbyt
Copy link
Contributor Author

Absorbed by #13536

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dns Issues and PRs related to the dns subsystem. repl Issues and PRs related to the REPL subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants