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.

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

Chore: Don't require an issue reference in check-commit npm script #7104

Merged
merged 1 commit into from Sep 12, 2016

Conversation

not-an-aardvark
Copy link
Member

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix ([template](https://github.com/eslint/eslint/blob/master/templates/bug-report.md))
[ ] New rule ([template](https://github.com/eslint/eslint/blob/master/templates/rule-proposal.md))
[ ] Changes an existing rule ([template](https://github.com/eslint/eslint/blob/master/templates/rule-change-proposal.md))
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:

If the item you've check above has a template, please paste the template questions here and answer them.

Please check each item to ensure your pull request is ready:

  • I've read the pull request guide
  • I've included tests for my change
  • I've updated documentation for my change (if appropriate)

What changes did you make? (Give an overview)

This removes the check for an issue reference in the npm run check-commit script, since that is no longer required by eslintbot or the docs. (discussed in Gitter)

Is there anything you'd like reviewers to focus on?

Nothing in particular.

@eslintbot
Copy link

LGTM

@platinumazure
Copy link
Member

For any reviewers: I had recommended submitting the PR with no tests because there don't seem to be existing tests. If we want tests around Makefile.js functionality, my preference would be that we just include checkGitCommit tests in this PR and create a separate issue/PR for adding test coverage elsewhere in that file.

@platinumazure
Copy link
Member

@not-an-aardvark Thanks for submitting this! Unfortunately you need to remove the ISSUE_REGEX declaration as well since the variable is unused. I'll take another look when Travis and AppVeyor are passing 😄

@eslintbot
Copy link

LGTM

@not-an-aardvark
Copy link
Member Author

Whoops, that's what I get for not running the tests locally. Fixed.

@platinumazure
Copy link
Member

platinumazure commented Sep 10, 2016

LGTM, waiting for others to review.

@nzakas
Copy link
Member

nzakas commented Sep 12, 2016

LGTM. Thanks!

@nzakas nzakas merged commit 9a2aefb into eslint:master Sep 12, 2016
@not-an-aardvark not-an-aardvark deleted the fix-npm-run-check-commit branch September 12, 2016 18:26
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants