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

Include period at end of no-unused-vars message #6739

Merged
merged 1 commit into from Jul 24, 2016

Conversation

wavebeem
Copy link
Contributor

What issue does this pull request address?

Issue #6738

What changes did you make? (Give an overview)

I added a period to the end of the message for no-unused-vars.

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

The other rules in ESLint seem to end their messages with a period, but no-unused-vars doesn't, so this PR attempts to change that.

@eslintbot
Copy link

Thanks for the pull request, @wavebeem! I took a look to make sure it's ready for merging and found some changes are needed:

  • The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.
  • Pull requests with code require an issue to be mentioned at the end of the commit summary, such as (fixes #1234). Please update the commit summary with an issue (file a new issue if one doesn't already exist).

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

@mention-bot
Copy link

@wavebeem, thanks for your PR! By analyzing the annotation information on this pull request, we identified @vitorbal, @mysticatea and @markelog to be potential reviewers

@eslintbot
Copy link

LGTM

@markelog
Copy link
Member

You need to change tests, they also check message.

Also, it seems there is quite a few rules with messages like that, would you like to fix them all?

@wavebeem
Copy link
Contributor Author

@markelog I tried looking for tests that checked the message, but apparently my search didn't turn them up. I can take a look maybe later today or tomorrow at the other messages and fixing the tests.

@nzakas
Copy link
Member

nzakas commented Jul 23, 2016

The tests are in tests/lib/rules/no-unused-vars.js

@nzakas
Copy link
Member

nzakas commented Jul 23, 2016

@markelog we should limit this PR to the original issue. If you're seeing problems with other rules, please open a new issue (it's ok to have one issue for multiple rules, we just shouldn't expand the scope of an already existing issue that is already being worked on).

@wavebeem
Copy link
Contributor Author

Thanks for the heads up on the location. I think I should have time today
to fix up the tests.

And good call on not expanding the current issue. Does it seem reasonable
to have a second issue after this encompassing the change to make all
messages consistently have ending punctuation?

On Sat, Jul 23, 2016 at 10:29 AM, Nicholas C. Zakas <
notifications@github.com> wrote:

@markelog https://github.com/markelog we should limit this PR to the
original issue. If you're seeing problems with other rules, please open a
new issue (it's ok to have one issue for multiple rules, we just shouldn't
expand the scope of an already existing issue that is already being worked
on).


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6739 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAKyr1P4T5J9Jq4nLO-ffCT91d-KrUsZks5qYk9vgaJpZM4JSbn0
.

@markelog
Copy link
Member

I would advice to fix all those messages in one commit in order to not clutter commit history

@wavebeem
Copy link
Contributor Author

Can I get consensus on which route to proceed with? Fixing just this one message, or fixing all of them. Thanks.

@platinumazure
Copy link
Member

Hi @wavebeem, I believe @nzakas was indicating that, for your original issue submission which was marked "accepted" while discussing one rule's issue, let's keep this PR focused to just that rule and not move the goalposts. If someone wants to create an issue for the remaining rules with this problem, then a future PR can address all of those at once.

I think you can just fix the tests for this issue, then we can merge. If you want to tackle the rest, feel free to open a new issue with a list of the other rules with this problem- we will definitely welcome your help again. This PR can be like a warm-up in that case :-)

The other rules in ESLint seem to end their messages with a period, but `no-unused-vars` doesn't, so this PR attempts to change that.
@eslintbot
Copy link

LGTM

@wavebeem
Copy link
Contributor Author

All right, I've fixed up all the failing tests, and eslint bot seems pleased as well. Lemme know what y'all think.

@gyandeeps
Copy link
Member

LGTM 👍
Thanks for contributing.

@gyandeeps gyandeeps merged commit e825458 into eslint:master Jul 24, 2016
@wavebeem
Copy link
Contributor Author

Thanks to all for such an amazing project!! Happy to give back.

On Jul 23, 2016, at 18:11, Gyandeep Singh notifications@github.com wrote:

LGTM 👍
Thanks for contributing.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@platinumazure
Copy link
Member

@wavebeem Thank you very much for contributing, we really appreciate it!

@nzakas
Copy link
Member

nzakas commented Jul 25, 2016

Sorry, was laid up for a bit, but just wanted to clarify: what @platinumazure said. :)

@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

8 participants