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

docs: Added information on how to run the linter. by diosney · Pull Request #7534 · nodejs/node · GitHub

/ node Public

docs: Added information on how to run the linter. - #7534

Closed
diosney wants to merge 6 commits into
nodejs:masterfrom
diosney:docs-adding-linter-command
ConversationCommits (6)ChecksFiles changed
Closed

docs: Added information on how to run the linter.#7534
diosney wants to merge 6 commits into
nodejs:masterfrom
diosney:docs-adding-linter-command

Conversation

diosney commented Jul 4, 2016

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

doc

Description of change

Right now there is no information in the building & testing docs on how to run the linter directly, which is useful so it can save time and separate tasks if one is checking only code style instead of checking the whole test suite.

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jul 4, 2016
diosney mentioned this pull request Jul 4, 2016
Closed
4 tasks
addaleax reviewed Jul 4, 2016
View reviewed changes
Comment thread CONTRIBUTING.md Outdated
Make sure the linter is happy and that all tests pass. Please, do not submit
patches that fail either check.

You can check if the linter is happy by running:

addaleax Jul 4, 2016

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

nit: I’d note that the linter is usually run as part of make test.

diosney Jul 4, 2016

Copy link
Copy Markdown
Contributor Author

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

@addaleax Agree, but if the tests are throwing errors that will disable you to properly lint the code.

addaleax Jul 4, 2016

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

@diosney Yep, I’ve seen the thread that prompted this PR, and I’m in favour of having the information of how to run only the linter noted here – I’d just want to save folks the trouble of thinking they have to run the linter separately under normal circumstances.

I don’t feel too strongly about it, and if you want to keep it this way, I won’t be in the way of that. :)

diosney Jul 4, 2016

Copy link
Copy Markdown
Contributor Author

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

@addaleax Oy! I think I understand you now! I just though you didn't see this addition as useful. Sorry for that.

I will add your proposal right now.

diosney Jul 4, 2016

Copy link
Copy Markdown
Contributor Author

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

@addaleax Just added your proposal, can you check it? Thanks!

addaleax commented Jul 4, 2016

Copy link
Copy Markdown
Member

LGTM, thanks!

jasnell commented Jul 4, 2016

Copy link
Copy Markdown
Member

LGTM

Trott commented Jul 4, 2016

Copy link
Copy Markdown
Member

LGTM. A few nits that can be ignored or dealt with:

  • I don't think linting is relevant to the BUILDING.md doc. So that change seems unnecessary.
  • test it directly might be better as run it directly since it is being contrasted with running tests.
  • Maybe mention that the linter won't run if any tests failed, since that was the particular gotcha that resulted in this PR. Ideally, there shouldn't be any failed tests, but if there are and you want to run the linter separately, here's how to do it...

diosney commented Jul 5, 2016

Copy link
Copy Markdown
Contributor Author

@Trott

[x] Removed: I don't think linting is relevant to the BUILDING.md doc. So that change seems unnecessary.

[x] Changed: test it directly might be better as run it directly since it is being contrasted with running tests.

[x] Added: Maybe mention that the linter won't run if any tests failed, since that was the particular gotcha that resulted in this PR. Ideally, there shouldn't be any failed tests, but if there are and you want to run the linter separately, here's how to do it...

Can you review the new changes?

cjihrig commented Jul 5, 2016

Copy link
Copy Markdown
Contributor

LGTM

diosney commented Jul 5, 2016

Copy link
Copy Markdown
Contributor Author

Sorry, reworded line. Tell me if is OK now.

Trott commented Jul 5, 2016

Copy link
Copy Markdown
Member

LGTM as is. Final nit (which as always, you can ignore): I would go with wording like this:

Running make test will run the linter as well unless one or more tests fail. If you want to run the linter without running tests, you can use make lint.

This fixes a few very minor things (e.g., consistent verb tense). However, if there are reasons to prefer the current wording, that's fine.

diosney commented Jul 5, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@Trott NP, changed to fix your nit.

Trott commented Jul 5, 2016

Copy link
Copy Markdown
Member

I know I said "final nit" before and I know this little bit is text I wrote, but I forgot that we try to avoid personal pronouns. So post-final nit: Get rid of you can, so:

you can use make lint

to

use make lint

Once again, with or without this change, LGTM!

diosney commented Jul 6, 2016

Copy link
Copy Markdown
Contributor Author

@Trott NP, I think this is how code review and PR moderating should be when a lot of people is contributing to the same project.

Fixed first post-final nit, expecting a post-post-final nit to be reported 😄

BTW, can you take a look at #7527 nit-wise?

Trott commented Jul 6, 2016

Copy link
Copy Markdown
Member

LGTM

addaleax commented Jul 10, 2016

Copy link
Copy Markdown
Member

Landed in fa46e50, thanks!

addaleax closed this Jul 10, 2016
addaleax pushed a commit that referenced this pull request Jul 10, 2016
doc: added information on how to run the linter.
fa46e50 
Added clarification about the linter execution.

PR-URL: #7534
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins mentioned this pull request Jul 11, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
doc: added information on how to run the linter.
7497174 
Added clarification about the linter execution.

PR-URL: #7534
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
doc: added information on how to run the linter.
eb841f9 
Added clarification about the linter execution.

PR-URL: #7534
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
doc: added information on how to run the linter.
f647f9a 
Added clarification about the linter execution.

PR-URL: #7534
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
doc: added information on how to run the linter.
f50dfcb 
Added clarification about the linter execution.

PR-URL: #7534
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins mentioned this pull request Jul 12, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
doc: added information on how to run the linter.
bb54aa7 
Added clarification about the linter execution.

PR-URL: #7534
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
doc: added information on how to run the linter.
700c6d9 
Added clarification about the linter execution.

PR-URL: #7534
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
evanlucas pushed a commit that referenced this pull request Jul 15, 2016
doc: added information on how to run the linter.
9e0cba0 
Added clarification about the linter execution.

PR-URL: #7534
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
evanlucas mentioned this pull request Jul 18, 2016
Merged
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

No reviews

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL