Skip to content

Update devDependencies.#2496

Merged
Trott merged 1 commit into
nodejs:masterfrom
XhmikosR:master-xmr-deps
Sep 5, 2019
Merged

Update devDependencies.#2496
Trott merged 1 commit into
nodejs:masterfrom
XhmikosR:master-xmr-deps

Conversation

@XhmikosR

@XhmikosR XhmikosR commented Sep 4, 2019

Copy link
Copy Markdown
Contributor

Please confirm that the nock changes don't require changes on our side; I checked the reply() calls and we don't seem to pass a function.

* nock 11.3.2
* handlebars 4.2.0
@Aissaoui-Ahmed Aissaoui-Ahmed requested review from a user, Trott and willin September 4, 2019 20:59
@Trott

Trott commented Sep 5, 2019

Copy link
Copy Markdown
Member

npm test fails for me but in the same way it does on the master branch, so
¯\(ツ)

@Trott

Trott commented Sep 5, 2019

Copy link
Copy Markdown
Member

npm run test:unit and npm run test:smoke both pass so I guess the nock update is OK?

@ghost

ghost commented Sep 5, 2019

Copy link
Copy Markdown

@Trott , I did a test locally by fetching the latest version of codes, and then change according to the post, run npm i and see the result. Everything seems fine with me, maybe you can remove node_modules and have another try ;)

image
image

@ghost

ghost commented Sep 5, 2019

Copy link
Copy Markdown

npm run test:unit and npm run test:smoke both pass so I guess the nock update is OK?

Everything seems fine with me. But since yours is of failure, maybe we want another to have a try? I'm running on windows 10, x86, latest version of upgration.

@Trott

Trott commented Sep 5, 2019

Copy link
Copy Markdown
Member

Looks like my problem is with a subdirectory's node_modules folder:

scripts/contributor-list/node_modules/winston/README.md: 840: MD001/heading-increment/header-increment Heading levels should only increment by one level at a time [Expected: h3; Actual: h4]

I can delete the node_modules folder to get it to pass but perhaps we ought to add an exclusion for node_modules?

Either way, this is not an error that's introduced in this PR, so this can land.

@Trott Trott merged commit 79d2df6 into nodejs:master Sep 5, 2019
@ghost

ghost commented Sep 5, 2019

Copy link
Copy Markdown

@Trott, node_modules seems to be excluded... Take this in '.gitignore'.

image

@XhmikosR

XhmikosR commented Sep 5, 2019

Copy link
Copy Markdown
Contributor Author

@Trott: node_modules should already be excluded, this is the command we have:

markdownlint \"**/*.md\" -i \"node_modules/\"

@XhmikosR XhmikosR deleted the master-xmr-deps branch September 5, 2019 07:20
@Trott

Trott commented Sep 5, 2019

Copy link
Copy Markdown
Member

@Trott, node_modules seems to be excluded... Take this in '.gitignore'.

image

@MaledongGit It's excluded in git, but that's not relevant here. That just means that someone (like me) who goes and builds that subdirectory from time to time will still have those files lying around and markdownlint still flags them.

@Trott

Trott commented Sep 5, 2019

Copy link
Copy Markdown
Member

@Trott: node_modules should already be excluded, this is the command we have:

markdownlint \"**/*.md\" -i \"node_modules/\"

@XhmikosR Apparently not. Try this:

  • Clean checkout of current master
  • npm ci
  • (cd scripts/contributor-list && npm install ) or cd scripts/contributor-list && npm install && cd .. or Windows equivalent
  • npm run test:lint (or Windows equivalent)

Errors... My guess is that without explicit globbing, the -i option assumes the path is relative to PWD

@XhmikosR

XhmikosR commented Sep 5, 2019

Copy link
Copy Markdown
Contributor Author

We should make it a glob then. Personally, I wasn't aware of this case.

@Trott

Trott commented Sep 5, 2019

Copy link
Copy Markdown
Member

We should make it a glob then. Personally, I wasn't aware of this case.

Testing and globbing isn't fixing it. Might be a straight up bug. Or maybe -i needs to precede the main argument/target.

@Trott

Trott commented Sep 5, 2019

Copy link
Copy Markdown
Member

This fixes it:

markdownlint  -i "**/node_modules/**" "**/*.md"

@XhmikosR

XhmikosR commented Sep 5, 2019

Copy link
Copy Markdown
Contributor Author

LGTM, I was about to check it out myself.

@Trott

Trott commented Sep 5, 2019

Copy link
Copy Markdown
Member

PR: #2507

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants