GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: remove common.noop by Trott · Pull Request #12822 · nodejs/node

Conversation

@Trott

@Trott Trott commented May 4, 2017

Copy link
Copy Markdown
Member

This change removes common.noop from the Node.js internal testing
common module.

Over the last few weeks, I've grown to dislike the common.noop
abstraction.

First, new (and experienced) contributors are unaware of it and so it
results in a large number of low-value nits on PRs. It also increases
the number of things newcomers and infrequent contributors have to be
aware of to be effective on the project.

Second, it is confusing. Is it a singleton/property or a getter? Which
should be expected? This can lead to subtle and hard-to-find bugs. (To
my knowledge, none have landed on master. But I also think it's only a
matter of time.)

Third, the abstraction is low-value in my opinion. What does it really
get us? A case could me made that it is without value at all.

Lastly, and this is minor, but the abstraction is wordier than not using
the abstraction. common.noop doesn't save anything over () => {}.

So, I propose removing it.

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

test

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Trott Trott added the test Issues and PRs related to Node.js core tests and test infrastructure. label May 4, 2017
@nodejs-github-bot nodejs-github-bot added addons Issues and PRs related to native addons. node-api Issues and PRs related to Node-API. test Issues and PRs related to Node.js core tests and test infrastructure. labels May 4, 2017
@refack

refack commented May 4, 2017

Copy link
Copy Markdown
Contributor

I don't remember who said that Function.prototype.call is a built in noop for when you need to refer to it by name (removeListener, etc)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
not-an-aardvark
not-an-aardvark approved these changes May 4, 2017
View reviewed changes
@TimothyGu

TimothyGu commented May 4, 2017

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/7846/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
TimothyGu
TimothyGu approved these changes May 4, 2017
View reviewed changes

@TimothyGu TimothyGu left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vsemozhetbyt vsemozhetbyt mentioned this pull request May 4, 2017
test: reduce string concatenations #12735
Closed
3 of 24 tasks
benjamingr
benjamingr approved these changes May 4, 2017
View reviewed changes
@jasnell

jasnell commented May 4, 2017

Copy link
Copy Markdown
Member

I'm going to abstain on this. I find it useful (which is why I added it :-) ...). I would likely just do it as a revert of the original commit that added it then do a separate commit that adds back in the changes to common.mustCall() defaulting to a no-op.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lpinca

lpinca commented May 4, 2017

Copy link
Copy Markdown
Member

Most of the problems caused by common.noop can be avoided by paying a little attention when using it so I'm with @jasnell in the sense that I find it useful but I'm also fine with not using it at all :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr

benjamingr commented May 4, 2017

Copy link
Copy Markdown
Member

Note that I reviewed the changes and the changes LGTM, but I'm +0 on actually removing common.noop.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aqrln aqrln mentioned this pull request May 4, 2017
test: make common.noop a getter #12732
Closed
4 tasks done
@lpinca lpinca mentioned this pull request May 5, 2017
tls: accept lookup option for tls.connect() #12839
Closed
4 tasks done
@Trott
Trott force-pushed the nononoop branch from bafc989 to 33eba38 Compare May 5, 2017 20:19
@Trott

Trott commented May 5, 2017

Copy link
Copy Markdown
Member Author

Rebased to resolve a few conflicts. New CI: https://ci.nodejs.org/job/node-test-pull-request/7907/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented May 6, 2017

Copy link
Copy Markdown
Member Author

Obviously, I"m all for this, but there are more neutral comments than supportive comments. If anyone who hasn't weighed in yet has objections, please comment! Thanks! /cc @nodejs/testing
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gibfahn
gibfahn approved these changes May 6, 2017
View reviewed changes
addaleax
addaleax approved these changes May 7, 2017
View reviewed changes
@thefourtheye

thefourtheye commented May 8, 2017

Copy link
Copy Markdown
Contributor

-0. I kinda liked the idea of common.noop. If we are repeating something in many files, it is better to put it in a common module. Moreover, if we want to use normal functions instead of arrow functions for noop, for whatever reason, again there will be code churn.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Trott force-pushed the nononoop branch from 33eba38 to 7972beb Compare May 10, 2017 02:14
@Trott

Trott commented May 10, 2017

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/7981/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Trott force-pushed the nononoop branch from 7972beb to 6ad4ea3 Compare May 10, 2017 02:33
@Trott

Trott commented May 10, 2017

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/7982/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Trott force-pushed the nononoop branch from 6ad4ea3 to c358923 Compare May 10, 2017 21:01
@Trott

Trott commented May 10, 2017

Copy link
Copy Markdown
Member Author

Rebased. Another CI: https://ci.nodejs.org/job/node-test-pull-request/7998/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Trott force-pushed the nononoop branch 3 times, most recently from 37d072e to 7bf77f7 Compare May 17, 2017 04:49
@Trott
Trott force-pushed the nononoop branch 12 times, most recently from 28b1d2b to ff6739c Compare June 16, 2017 16:45
@Trott
Trott force-pushed the nononoop branch 3 times, most recently from adadb15 to 7465731 Compare June 18, 2017 15:42
@Trott
test: remove common.noop
d797b19 
This change removes `common.noop` from the Node.js internal testing
common module.

Over the last few weeks, I've grown to dislike the `common.noop`
abstraction.

First, new (and experienced) contributors are unaware of it and so it
results in a large number of low-value nits on PRs. It also increases
the number of things newcomers and infrequent contributors have to be
aware of to be effective on the project.

Second, it is confusing. Is it a singleton/property or a getter? Which
should be expected? This can lead to subtle and hard-to-find bugs. (To
my knowledge, none have landed on master. But I also think it's only a
matter of time.)

Third, the abstraction is low-value in my opinion. What does it really
get us? A case could me made that it is without value at all.

Lastly, and this is minor, but the abstraction is wordier than not using
the abstraction. `common.noop` doesn't save anything over `() => {}`.

So, I propose removing it.
@Trott

Trott commented Jul 3, 2017

Copy link
Copy Markdown
Member Author

It's time...

CI: https://ci.nodejs.org/job/node-test-pull-request/8939/
benjamingr reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gibfahn
gibfahn approved these changes Jul 3, 2017
View reviewed changes
@Trott

Trott commented Jul 3, 2017

Copy link
Copy Markdown
Member Author

Landed in 380929e
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax

addaleax commented Jul 11, 2017

Copy link
Copy Markdown
Member

@Trott This doesn’t land cleanly on 8.x; can you backport it? (link to the backport guide)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott Trott mentioned this pull request Jul 11, 2017
[v8.x backport] test: remove common.noop #14174
Closed
2 tasks done
@Trott

Trott commented Jul 11, 2017

Copy link
Copy Markdown
Member Author

Backport in #14174
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax mentioned this pull request Jul 18, 2017
v8.2.0 proposal #13744
Merged
@MylesBorins

MylesBorins commented Aug 16, 2017

Copy link
Copy Markdown
Contributor

Managed to get this to land on v6.x

this common.noop thing has been a trip, just backported it yesterday 😅
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
v6.11.3-proposal #14852
Merged
This was referenced Sep 5, 2017
2017-09-05 Version 6.11.3 'Boron' (LTS) @MylesBorins beevelop/docker-android-nodejs#139
Closed
2017-09-05 Version 6.11.3 'Boron' (LTS) @MylesBorins beevelop/docker-android-nodejs#140
Closed
2017-09-05 Version 6.11.3 'Boron' (LTS) @MylesBorins beevelop/docker-android-nodejs#141
Closed
2017-09-05 Version 6.11.3 'Boron' (LTS) @MylesBorins beevelop/docker-android-nodejs#142
Closed
2017-09-05 Version 6.11.3 'Boron' (LTS) @MylesBorins beevelop/docker-android-nodejs#143
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes
@benjamingr benjamingr benjamingr approved these changes
+4 more reviewers
@refack refack refack approved these changes
@TimothyGu TimothyGu TimothyGu approved these changes
@not-an-aardvark not-an-aardvark not-an-aardvark approved these changes
@gibfahn gibfahn gibfahn approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

addons Issues and PRs related to native addons. node-api Issues and PRs related to Node-API. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

12 participants

@Trott @refack @TimothyGu @jasnell @lpinca @benjamingr @thefourtheye @gibfahn @addaleax @MylesBorins @not-an-aardvark @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.