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.

Sign up for GitHub

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

Jump to bottom

tools: update eslint-plugin-markdown up to 1.0.0-beta.7 #14047

Closed
wants to merge 3 commits into from
Closed

tools: update eslint-plugin-markdown up to 1.0.0-beta.7 #14047

wants to merge 3 commits into from

Conversation

vsemozhetbyt
Copy link
Contributor

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

tools, doc

Commit 1:

tools: remove pinning of eslint-plugin-markdown

An issue affecting Node.js source has been fixed in
eslint-plugin-markdown so we don't need to pin it to beta-4 anymore.

Refs: https://github.com/eslint/eslint-plugin-markdown/issues/69

Commit 2:

tools: update: [email protected]

Commit 3:

doc: fix for [email protected]

Refs:
eslint/markdown#69
eslint/markdown#73
https://github.com/eslint/eslint-plugin-markdown#skipping-blocks
#14045

@vsemozhetbyt
tools: remove pinning of eslint-plugin-markdown
6c4bb4a 
An issue affecting Node.js source has been fixed in
eslint-plugin-markdown so we don't need to pin it to beta-4 anymore.

Refs: eslint/markdown#69
@vsemozhetbyt
tools: update: [email protected]
4f6b7b3
@vsemozhetbyt
doc: fix for [email protected]
d4769fe
@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Jul 3, 2017
@vsemozhetbyt vsemozhetbyt added the doc Issues and PRs related to the documentations. label Jul 3, 2017
@vsemozhetbyt
Copy link
Contributor Author

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

@vsemozhetbyt vsemozhetbyt mentioned this pull request Jul 3, 2017
Closed
2 tasks
vsemozhetbyt
vsemozhetbyt commented Jul 3, 2017
View reviewed changes
doc/api/dns.md
```js
[ { type: 'A', address: '127.0.0.1', ttl: 299 },
{ type: 'CNAME', value: 'example.com' },
{ type: 'MX', exchange: 'alt4.aspmx.l.example.com', priority: 50 },
{ type: 'NS', value: 'ns1.example.com', type: 'NS' },
{ type: 'NS', value: 'ns1.example.com' },
Copy link
Contributor Author

@vsemozhetbyt vsemozhetbyt Jul 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This fix illustrates why we should use rule-disabling instead of full disabling when it is possible.

@vsemozhetbyt
Copy link
Contributor Author

All commits should be squashed before landing as they are not self-sufficient (the second fails linting without the third) and are separated for easier reviewing.

Trott
Trott approved these changes Jul 3, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Assuming a clean lint run on CI: First and last commits LGTM. Middle commit rubber-stamp LGTM.

hiroppy
hiroppy approved these changes Jul 3, 2017
View reviewed changes
Copy link
Member

@hiroppy hiroppy left a comment

Choose a reason for hiding this comment

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

LGTM👍

lpinca
lpinca approved these changes Jul 3, 2017
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

Rubber stamp LGTM

This was referenced Jul 3, 2017
Closed
Closed
@vsemozhetbyt
Copy link
Contributor Author

There is an issue in 1.0.0-beta.7:

eslint/markdown#76

Our code is unaffected (we always have an empty line before such comments), but we should better track the fixing and update to reduce possible confusing.

@vsemozhetbyt
Copy link
Contributor Author

Landed in 15599cb

@vsemozhetbyt vsemozhetbyt deleted the update-eslint-plugin-markdown branch July 5, 2017 03:56
@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Jul 5, 2017 •
edited
Loading

@nodejs/collaborators You may need to rebase your open doc PRs with code fragments after this and rerun Linter CI.

addaleax pushed a commit that referenced this pull request Jul 11, 2017
@vsemozhetbyt @addaleax
tools: update: [email protected]
b22cf28 
* Remove pinning of eslint-plugin-markdown

  An issue affecting Node.js source has been fixed in
  eslint-plugin-markdown so we don't need to pin it to beta-4 anymore.

  Refs: eslint/markdown#69

* Update eslint-plugin-markdown up to 1.0.0-beta.7

* Fix docs for [email protected]

PR-URL: #14047
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@addaleax addaleax mentioned this pull request Jul 11, 2017
Merged
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@vsemozhetbyt @addaleax
tools: update: [email protected]
aa4a700 
* Remove pinning of eslint-plugin-markdown

  An issue affecting Node.js source has been fixed in
  eslint-plugin-markdown so we don't need to pin it to beta-4 anymore.

  Refs: eslint/markdown#69

* Update eslint-plugin-markdown up to 1.0.0-beta.7

* Fix docs for [email protected]

PR-URL: #14047
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
@vsemozhetbyt @Fishrock123
tools: update: [email protected]
3bc72ab 
* Remove pinning of eslint-plugin-markdown

  An issue affecting Node.js source has been fixed in
  eslint-plugin-markdown so we don't need to pin it to beta-4 anymore.

  Refs: eslint/markdown#69

* Update eslint-plugin-markdown up to 1.0.0-beta.7

* Fix docs for [email protected]

PR-URL: #14047
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 21, 2017
@vsemozhetbyt @MylesBorins
build, doc, tools: add eslint-plugin-markdown
ce4718f 
* Install [email protected]
* 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 <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 21, 2017
@vsemozhetbyt @MylesBorins
build, doc, tools: add eslint-plugin-markdown
8fef985 
* Install [email protected]
* 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 <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 31, 2017
@vsemozhetbyt @MylesBorins
build, doc, tools: add eslint-plugin-markdown
e5ca046 
* Install [email protected]
* 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 <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hiroppy hiroppy hiroppy approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@vsemozhetbyt @refack @Trott @lpinca @cjihrig @hiroppy @MylesBorins @nodejs-github-bot