FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc, tools: add doc linting to CI by vsemozhetbyt · Pull Request #12640 · nodejs/node · GitHub

/ node Public

doc, tools: add doc linting to CI - #12640

Closed
vsemozhetbyt wants to merge 1 commit into
nodejs:masterfrom
vsemozhetbyt:doc-lint-ci
ConversationCommits (1)ChecksFiles changed
Closed

doc, tools: add doc linting to CI#12640
vsemozhetbyt wants to merge 1 commit into
nodejs:masterfrom
vsemozhetbyt:doc-lint-ci

Conversation

vsemozhetbyt commented Apr 25, 2017

Copy link
Copy Markdown
Contributor
Checklist
Affected core subsystem(s)

doc, tools

Fixes: #12635
Refs: #12563

nodejs-github-bot added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform. labels Apr 25, 2017
vsemozhetbyt added the doc Issues and PRs related to the documentations. label Apr 25, 2017

vsemozhetbyt commented Apr 25, 2017

Copy link
Copy Markdown
Contributor Author

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

vsemozhetbyt mentioned this pull request Apr 25, 2017
Closed

vsemozhetbyt commented Apr 25, 2017

Copy link
Copy Markdown
Contributor Author

Just 1 Windows machine unstable in CI.

For a test, I shall add an error in doc and run a new CI.

vsemozhetbyt commented Apr 25, 2017

Copy link
Copy Markdown
Contributor Author

CI that SHOULD FAIL: https://ci.nodejs.org/job/node-test-pull-request/7666/

vsemozhetbyt commented Apr 25, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Whohoo! Linter fails.

Do I get it right that linter in CI is run only as a separate job and is not run in builds? So we can test only Makefile here and not vcbuild.bat?

vsemozhetbyt commented Apr 25, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

It seems so, as only the linter job fails. Reverting the error...

vsemozhetbyt commented Apr 25, 2017

Copy link
Copy Markdown
Contributor Author

Linter CI: https://ci.nodejs.org/job/node-test-linter/8549/

vsemozhetbyt commented Apr 25, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Please, let me know if this can be landed without delay as a follow-up fix for #12563. Currently, we have a conflict between local and CI linting. We can land commits that will break ESLint runs in local buildings.

cc @not-an-aardvark, @silverwind, @Trott

vsemozhetbyt mentioned this pull request Apr 25, 2017
Closed
3 tasks
not-an-aardvark approved these changes Apr 25, 2017
View reviewed changes

not-an-aardvark left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The changes LGTM, but I'll defer to others about whether this should be landed without waiting 48 hours.

mhdawson approved these changes Apr 25, 2017
View reviewed changes

mhdawson left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

gibfahn commented Apr 25, 2017

Copy link
Copy Markdown
Member

+1 to landing sooner to avoid CI/local linting issues

vsemozhetbyt commented Apr 25, 2017

Copy link
Copy Markdown
Contributor Author

I will dare to land now having got a one +1 and some LGTM.

vsemozhetbyt commented Apr 25, 2017

Copy link
Copy Markdown
Contributor Author

Landed in 42dca99

vsemozhetbyt deleted the doc-lint-ci branch April 25, 2017 19:47
vsemozhetbyt added a commit that referenced this pull request Apr 25, 2017
doc, tools: add doc linting to CI
42dca99 
PR-URL: #12640
Fixes: #12635
Refs: #12563
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

vsemozhetbyt commented Apr 25, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

It seems all the old PRs will still use the old linting in CI if not rebased.

The same example: #12549 (comment)

New linter CI was launched after landing this PR, but console log shows old linting commands: https://ci.nodejs.org/job/node-test-linter/8559/console

I think this is an additional reason for having this landed sooner.

evanlucas mentioned this pull request May 1, 2017
Merged
evanlucas pushed a commit that referenced this pull request May 1, 2017
doc, tools: add doc linting to CI
41cfe64 
PR-URL: #12640
Fixes: #12635
Refs: #12563
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
doc, tools: add doc linting to CI
47faa02 
PR-URL: #12640
Fixes: #12635
Refs: #12563
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
vsemozhetbyt mentioned this pull request May 16, 2017
Closed
2 tasks

gibfahn commented May 16, 2017

Copy link
Copy Markdown
Member

Will be backported to v6.x with #12563 if that lands.

jasnell mentioned this pull request May 11, 2017
Closed
gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
vsemozhetbyt mentioned this pull request Jul 4, 2017
Closed
3 tasks
MylesBorins pushed a commit that referenced this pull request Jul 21, 2017
build, doc, tools: add eslint-plugin-markdown
ce4718f 
* Install eslint-plugin-markdown@1.0.0-beta.7
* Add doc/.eslintrc.yaml
* Add `plugins: [markdown]` to the main .eslintrc.yaml
* .js files in doc folder added to .eslintignore
* Update Makefile, vcbuild.bat, and tools/jslint.js

Refs: #12563
Refs: #12640
Refs: #14047

PR-URL: #14067
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 21, 2017
build, doc, tools: add eslint-plugin-markdown
8fef985 
* Install eslint-plugin-markdown@1.0.0-beta.7
* Add doc/.eslintrc.yaml
* Add `plugins: [markdown]` to the main .eslintrc.yaml
* .js files in doc folder added to .eslintignore
* Update Makefile, vcbuild.bat, and tools/jslint.js

Refs: #12563
Refs: #12640
Refs: #14047

PR-URL: #14067
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 31, 2017
build, doc, tools: add eslint-plugin-markdown
e5ca046 
* Install eslint-plugin-markdown@1.0.0-beta.7
* Add doc/.eslintrc.yaml
* Add `plugins: [markdown]` to the main .eslintrc.yaml
* .js files in doc folder added to .eslintignore
* Update Makefile, vcbuild.bat, and tools/jslint.js

Refs: #12563
Refs: #12640
Refs: #14047

PR-URL: #14067
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
aliscco mentioned this pull request Apr 27, 2023
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

cjihrig cjihrig approved these changes
+3 more reviewers
mhdawson mhdawson approved these changes
not-an-aardvark not-an-aardvark approved these changes
gibfahn gibfahn approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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. windows Issues and PRs related to the Windows platform.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL