Skip to content

tools: force common be required before any other modules - #27650

Closed
ZYSzys wants to merge 2 commits into
nodejs:masterfrom
zys-contrib:eslint-require-common
Closed

tools: force common be required before any other modules#27650
ZYSzys wants to merge 2 commits into
nodejs:masterfrom
zys-contrib:eslint-require-common

Conversation

@ZYSzys

@ZYSzysZYSzys commented May 11, 2019

Copy link
Copy Markdown
Member

Add a new eslint rule require-common-first to enforce common be loaded in tests before any other modules.

The common module should be required in tests before any other module: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#lines-1-3

Refs: #27455 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-botnodejs-github-bot added the tools Issues and PRs related to the tools directory. label May 11, 2019
@nodejs-github-bot

This comment has been minimized.

@lpincalpincaMay 11, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Doesn't this change the original rule? It could have been configured to enforce multiple modules before, now it only supports a single one.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Theoretically yes, but the usage of this rule is only for the common module under test directory, so it wouldn't break our linter and I think maybe it acceptable ?

## common module is mandatory in tests
node-core/required-modules: [error, common]

I'm also not sure to limit this rule for a single one module configuration, or maybe should I write a new rule(maybe named require-top-level-common) to force common be required before any other modules ?

Any advice ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think a new rule makes sense or also change this rule name as required-modules is misleading.

cc: @Trott

@TrottTrottMay 12, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm fine with the approach in this pull request + a name change to required-module.

Alternatively, can we add a configuration object to the rule that allows you to specify that one of the modules must be loaded before all the others and only do this "load first" check for the module that has that option set?? It might look like this:

node-core/required-modules: [error, [common, assert, foo], { first: common}]

That might be over-engineering or it might be good flexibility to build in for the future. I'm fine with either approach.

@ZYSzysZYSzysMay 13, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yup 👍, that sounds great, will try to implement this in the next PR.

Comment threadtools/eslint-rules/require-common-first.js Outdated
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 13, 2019
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@ZYSzys

Copy link
Copy Markdown
MemberAuthor

Landed in dcc5e51

@ZYSzysZYSzys closed this May 13, 2019
ZYSzys added a commit that referenced this pull request May 13, 2019
PR-URL: #27650
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@ZYSzys
ZYSzys deleted the eslint-require-common branch May 13, 2019 12:02
targos pushed a commit that referenced this pull request May 13, 2019
PR-URL: #27650
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeARBridgeAR mentioned this pull request May 21, 2019
4 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.toolsIssues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@ZYSzys@nodejs-github-bot@Trott@lpinca@BridgeAR