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

doc: fix linting command for vcbuild #11151

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
@Trott
Member

Trott commented Feb 3, 2017

Currently, vcbuild only supports jslint. vcbuild lint will not
work because there is no lint task specified in vcbuild.bat. Update
documentation to use vcbuild jslint instead.

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

@Trott Trott added the doc label Feb 3, 2017

@seishun

This comment has been minimized.

Show comment
Hide comment
@seishun

seishun Feb 3, 2017

Member

Might want to mention that vcbuild jslint only does part of what make lint does.

Member

seishun commented Feb 3, 2017

Might want to mention that vcbuild jslint only does part of what make lint does.

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Feb 3, 2017

Contributor

Why not add vcbuild lint?

Contributor

silverwind commented Feb 3, 2017

Why not add vcbuild lint?

@seishun

This comment has been minimized.

Show comment
Hide comment
@seishun

seishun Feb 3, 2017

Member

@silverwind it seems it's not so simple: #5514 (comment)

But sure, it would be a better solution.

Member

seishun commented Feb 3, 2017

@silverwind it seems it's not so simple: #5514 (comment)

But sure, it would be a better solution.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Feb 3, 2017

Member

Might want to mention that vcbuild jslint only does part of what make lint does.

Done.

Member

Trott commented Feb 3, 2017

Might want to mention that vcbuild jslint only does part of what make lint does.

Done.

@seishun

seishun approved these changes Feb 3, 2017

LGTM unless someone implements vcbuild lint.

Show outdated Hide outdated CONTRIBUTING.md
`make lint`/`vcbuild lint`.
`make lint`/`vcbuild jslint`. (At this time, only JavaScript linting is
available on Windows. `make lint` on POSIX will run both JavaScript linting and
C++ linting.)

This comment has been minimized.

@seishun

seishun Feb 3, 2017

Member

I don't like how the text between brackets is longer than the text it's supposed to clarify. Maybe it's better just mention lint and jslint separately.

@seishun

seishun Feb 3, 2017

Member

I don't like how the text between brackets is longer than the text it's supposed to clarify. Maybe it's better just mention lint and jslint separately.

This comment has been minimized.

@jasnell

jasnell Feb 3, 2017

Member

Perhaps just remove the parens?

@jasnell

jasnell Feb 3, 2017

Member

Perhaps just remove the parens?

This comment has been minimized.

@Trott

Trott Feb 3, 2017

Member

Sure, parens removed.

@Trott

Trott Feb 3, 2017

Member

Sure, parens removed.

@mscdex mscdex added build tools labels Feb 3, 2017

Show outdated Hide outdated CONTRIBUTING.md
`make lint`/`vcbuild lint`.
`make lint`/`vcbuild jslint`. (At this time, only JavaScript linting is
available on Windows. `make lint` on POSIX will run both JavaScript linting and
C++ linting.)

This comment has been minimized.

@jasnell

jasnell Feb 3, 2017

Member

Perhaps just remove the parens?

@jasnell

jasnell Feb 3, 2017

Member

Perhaps just remove the parens?

doc: fix linting command for vcbuild
Currently, `vcbuild` only supports `jslint`. `vcbuild lint` will not
work because there is no `lint` task specified in `vcbuild.bat`. Update
documentation to use `vcbuild jslint` instead.
@lpinca

lpinca approved these changes Feb 4, 2017

@jasnell

jasnell approved these changes Feb 4, 2017

Trott added a commit to Trott/io.js that referenced this pull request Feb 6, 2017

doc: fix linting command for vcbuild
Currently, `vcbuild` only supports `jslint`. `vcbuild lint` will not
work because there is no `lint` task specified in `vcbuild.bat`. Update
documentation to use `vcbuild jslint` instead.

PR-URL: nodejs#11151
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Feb 6, 2017

Member

Landed in dccd97d

Member

Trott commented Feb 6, 2017

Landed in dccd97d

@Trott Trott closed this Feb 6, 2017

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 9, 2017

doc: fix linting command for vcbuild
Currently, `vcbuild` only supports `jslint`. `vcbuild lint` will not
work because there is no `lint` task specified in `vcbuild.bat`. Update
documentation to use `vcbuild jslint` instead.

PR-URL: nodejs#11151
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

doc: fix linting command for vcbuild
Currently, `vcbuild` only supports `jslint`. `vcbuild lint` will not
work because there is no `lint` task specified in `vcbuild.bat`. Update
documentation to use `vcbuild jslint` instead.

PR-URL: nodejs#11151
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

KryDos added a commit to KryDos/node that referenced this pull request Feb 25, 2017

doc: fix linting command for vcbuild
Currently, `vcbuild` only supports `jslint`. `vcbuild lint` will not
work because there is no `lint` task specified in `vcbuild.bat`. Update
documentation to use `vcbuild jslint` instead.

PR-URL: nodejs#11151
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 7, 2017

Member

Would need backport PRs to land on v4 or v6

Member

jasnell commented Mar 7, 2017

Would need backport PRs to land on v4 or v6

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