Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tools: rolled up lint-md #20109

Merged
merged 5 commits into from
Sep 11, 2018
Merged

tools: rolled up lint-md #20109

merged 5 commits into from
Sep 11, 2018

Conversation

refack
Copy link
Contributor

@refack refack commented Apr 17, 2018

replace remark-cli and remark-preset-lint-node with a "rolled-up" single file.

  • Should solve all build & Makefile issue
  • very size economic (17KB instead of 20MB ~1300 files)
  • includes all recipes for re rolling & updating

/CC @wooorm @nodejs/build
H/T: @rollup

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

@refack refack added wip Issues and PRs that are still a work in progress. doc Issues and PRs related to the documentations. build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. labels Apr 17, 2018
@refack refack self-assigned this Apr 17, 2018
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Apr 17, 2018
@refack
Copy link
Contributor Author

refack commented Apr 17, 2018

@@ -0,0 +1,2 @@
/node_modules/
/dist/
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: missing line break at EOF.

@@ -1,22 +0,0 @@
(The MIT License)
Copy link
Member

Choose a reason for hiding this comment

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

Please could you update

addlicense "remark-cli" "tools/remark-cli" "$(cat ${rootdir}/tools/remark-cli/LICENSE)"
?

@gibfahn
Copy link
Member

gibfahn commented Apr 18, 2018

This sounds like a great solution to the problem. If it works we should probably consider doing the same for eslint.

},
"scripts": {
"build": "rollup -c",
"build-node": "npm run build && cp dist/* .."
Copy link
Member

@joyeecheung joyeecheung Apr 19, 2018

Choose a reason for hiding this comment

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

The lint-md-build makefile target is removed, can you add it back with a command that runs this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since our CI runs this for every lint, I've made make lint-md-build a no-op, and added make lint-md-rollup

@joyeecheung
Copy link
Member

Very nice work. Thank you for working on this!

@BridgeAR
Copy link
Member

BridgeAR commented Apr 25, 2018

This is marked as WIP @refack is there anything else to do to get this landed (besides open comments)?

@refack
Copy link
Contributor Author

refack commented Apr 27, 2018

refack is there anything else to do to get this landed (besides open comments)?

I was hoping for some feedback on how we should track the license, and document this. Also i'm not sure how to test the new script...

@joyeecheung
Copy link
Member

@refack I think writing a bad .md in the root directory then running make lint-md expecting a non-zero exit code and some output complaining about it should be enough. Can be part of the doctool test suite. If we don't want to always run it we can just tweak the makefile and/or vcbuild.bat, it should be fine as long as it's run in the linter CI.

@BridgeAR
Copy link
Member

Ping @refack

@BridgeAR
Copy link
Member

I guess it is a good idea to do this but I am going to close this due to no response. If anyone feels like taking a stab at this, that would be great.

@refack please reopen in case you want to continue working on this.

@refack
Copy link
Contributor Author

refack commented Sep 6, 2018

Why should this not trigger the rollup?

Also lint-md-rollup uses npm up, and there is some spesific workaround code in the rollup config that could break:
https://github.com/nodejs/node/blob/67b6082e119b85551e5dcf4243b09f08f50f42ce/tools/node-lint-md-cli-rollup/rollup.config.js#L28-L36

@refack
Copy link
Contributor Author

refack commented Sep 7, 2018

PR-URL: nodejs#20109
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#20109
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#20109
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#20109
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
* remove unused `tools/remark-cli`
* vcbuild tested with `vcbuild nobuild noprojgen lint-md-build lint-md`

PR-URL: nodejs#20109
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@refack refack merged commit e4dd213 into nodejs:master Sep 11, 2018
@refack
Copy link
Contributor Author

refack commented Sep 11, 2018

Landed in
d3442f2
2120dac
7fcc178
fe6c74c
e4dd213

@refack refack deleted the rolled-up-lint-md branch September 11, 2018 18:14
targos pushed a commit that referenced this pull request Sep 11, 2018
PR-URL: #20109
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this pull request Sep 11, 2018
PR-URL: #20109
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this pull request Sep 11, 2018
PR-URL: #20109
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this pull request Sep 11, 2018
PR-URL: #20109
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this pull request Sep 11, 2018
* remove unused `tools/remark-cli`
* vcbuild tested with `vcbuild nobuild noprojgen lint-md-build lint-md`

PR-URL: #20109
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos added a commit to targos/node that referenced this pull request Sep 21, 2018
- In release guide
- In Travis config

Refs: nodejs#20109
addaleax pushed a commit that referenced this pull request Sep 22, 2018
- In release guide
- In Travis config

Refs: #20109

PR-URL: #22991
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos added a commit that referenced this pull request Sep 23, 2018
- In release guide
- In Travis config

Refs: #20109

PR-URL: #22991
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@refack refack removed their assignment Oct 12, 2018
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
- In release guide
- In Travis config

Refs: #20109

PR-URL: #22991
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
- In release guide
- In Travis config

Refs: #20109

PR-URL: #22991
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants