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
Conversation
Thanks for the pull request, @wavebeem! I took a look to make sure it's ready for merging and found some changes are needed:
Can you please update the pull request to address these? (More information can be found in our pull request guide.) |
@wavebeem, thanks for your PR! By analyzing the annotation information on this pull request, we identified @vitorbal, @mysticatea and @markelog to be potential reviewers |
LGTM |
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? |
@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. |
The tests are in |
@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). |
Thanks for the heads up on the location. I think I should have time today And good call on not expanding the current issue. Does it seem reasonable On Sat, Jul 23, 2016 at 10:29 AM, Nicholas C. Zakas <
|
I would advice to fix all those messages in one commit in order to not clutter commit history |
Can I get consensus on which route to proceed with? Fixing just this one message, or fixing all of them. Thanks. |
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.
LGTM |
All right, I've fixed up all the failing tests, and eslint bot seems pleased as well. Lemme know what y'all think. |
LGTM 👍 |
Thanks to all for such an amazing project!! Happy to give back.
|
@wavebeem Thank you very much for contributing, we really appreciate it! |
Sorry, was laid up for a bit, but just wanted to clarify: what @platinumazure said. :) |
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.