Update: make `max-lines` report the actual number of lines (fixes #6766) #6764

jrencz · 2016-07-26T17:56:50Z

What issue does this pull request address?
It makes max-lines report the actual number of lines along with the limit.

What changes did you make? (Give an overview)
I altered the report message and updated max-lines test suite.

Is there anything you'd like reviewers to focus on?
Report output. May it be better?

Instead of reporting only that max value has been exceeded
it now also reports how many lines there is in the file.

mention-bot · 2016-07-26T17:56:52Z

@jrencz, thanks for your PR! By analyzing the annotation information on this pull request, we identified @alberto to be a potential reviewer

eslintbot · 2016-07-26T17:56:52Z

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

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.)

jquerybot · 2016-07-26T17:57:04Z

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

📝 Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.

If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

eslintbot · 2016-07-26T18:04:24Z

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

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.)

eslintbot · 2016-07-26T18:07:44Z

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

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.)

jrencz · 2016-07-28T09:16:19Z

@alberto may you please describe why "do not merge" was applied?

Most other PRs with "do not merge" have some kind of description.

I'm not sure if I have to do something more or you just have to think about this.

vitorbal · 2016-07-28T09:25:31Z

@jrencz thanks a lot for the PR. It was marked as "do not merge" simply because the proposal has not been accepted by the team yet.
As soon as the proposal is marked as accepted on the original issue, we'll remove the "do not merge" label from here, so nothing to do for now on your part.

jrencz · 2016-07-28T09:37:29Z

@vitorbal thank you

eslintbot · 2016-08-04T08:16:04Z

LGTM

vitorbal · 2016-08-04T08:41:32Z

tests/lib/rules/max-lines.js

@@ -21,11 +21,13 @@ const ruleTester = new RuleTester();

 /**
 * Returns the error message with the specified max number of lines
- * @param {number} lines Maximum number of lines
+ * @param {number} limitLines Maximum number of lines
+ * @param {number} actualLines Maximum number of lines


could you update the description of this param? Should be "Actual number of lines" I think

Right, my bad

vitorbal · 2016-08-04T12:47:47Z

LGTM, but waiting another day for others to take a look

…int#6766) Instead of reporting only that `max` value has been exceeded it now also reports how many lines there is in the file.

eslintbot · 2016-08-04T14:13:47Z

LGTM

gyandeeps · 2016-08-04T14:16:30Z

LGTM except I think we need to make the message little short.

vitorbal · 2016-08-05T08:39:09Z

lib/rules/max-lines.js

@@ -139,7 +139,11 @@ module.exports = {
                if (lines.length > max) {
                    context.report({
                        loc: { line: 1, column: 0 },
-                        message: "File must be at most " + max + " lines long."
+                        message: "File must be at most {{max}} lines long. It's {{actual}} lines long.",


@gyandeeps to address your concern: how about:

"File must be at most {{max}} lines long, was {{actual}}."

Although I don't feel like it's too long right now, do we have some guidelines on the maximum length of the error messages we should generate?

We don't have any guidelines, and I agree that it looks reasonably short to me.

ilyavolodin · 2016-08-09T00:59:21Z

LGTM. @gyandeeps Do you want to make message shorter or are you OK with merging this as is?

gyandeeps · 2016-08-09T01:01:53Z

that wasn't a very strong opinion. That was just my take but if other fine then i am fine. 😄

vitorbal · 2016-08-10T08:32:46Z

Alright, merging then. Thanks a lot for your contribution, @jrencz! 🎉

jquerybot added the CLA: Error label Jul 26, 2016

jrencz force-pushed the max-lines-report-actual-lines branch from f27f022 to 67c199c Compare July 26, 2016 18:04

jquerybot added CLA: Valid and removed CLA: Error labels Jul 26, 2016

jrencz force-pushed the max-lines-report-actual-lines branch from 67c199c to b90606a Compare July 26, 2016 18:07

jrencz changed the title ~~Update: make max-lines report the actual number of lines~~ Update: make max-lines report the actual number of lines (fixes #6766) Jul 26, 2016

alberto added the do not merge This pull request should not be merged yet label Jul 26, 2016

jrencz force-pushed the max-lines-report-actual-lines branch from b90606a to 807b20d Compare August 4, 2016 08:16

vitorbal reviewed Aug 4, 2016
View reviewed changes

vitorbal removed the do not merge This pull request should not be merged yet label Aug 4, 2016

Update: make max-lines report the actual number of lines (fixes esl…

62e0b01

…int#6766) Instead of reporting only that `max` value has been exceeded it now also reports how many lines there is in the file.

jrencz force-pushed the max-lines-report-actual-lines branch from 807b20d to 62e0b01 Compare August 4, 2016 14:13

vitorbal reviewed Aug 5, 2016
View reviewed changes

vitorbal merged commit d610d6c into eslint:master Aug 10, 2016

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: make `max-lines` report the actual number of lines (fixes #6766) #6764

Update: make `max-lines` report the actual number of lines (fixes #6766) #6764

jrencz commented Jul 26, 2016

mention-bot commented Jul 26, 2016

eslintbot commented Jul 26, 2016

jquerybot commented Jul 26, 2016

eslintbot commented Jul 26, 2016

eslintbot commented Jul 26, 2016

jrencz commented Jul 28, 2016

vitorbal commented Jul 28, 2016

jrencz commented Jul 28, 2016

eslintbot commented Aug 4, 2016

vitorbal Aug 4, 2016

jrencz Aug 4, 2016

vitorbal commented Aug 4, 2016

eslintbot commented Aug 4, 2016

gyandeeps commented Aug 4, 2016

vitorbal Aug 5, 2016 •

edited

ilyavolodin Aug 9, 2016

ilyavolodin commented Aug 9, 2016

gyandeeps commented Aug 9, 2016

vitorbal commented Aug 10, 2016

Update: make max-lines report the actual number of lines (fixes #6766) #6764

Update: make max-lines report the actual number of lines (fixes #6766) #6764

Conversation

jrencz commented Jul 26, 2016

mention-bot commented Jul 26, 2016

eslintbot commented Jul 26, 2016

jquerybot commented Jul 26, 2016

eslintbot commented Jul 26, 2016

eslintbot commented Jul 26, 2016

jrencz commented Jul 28, 2016

vitorbal commented Jul 28, 2016

jrencz commented Jul 28, 2016

eslintbot commented Aug 4, 2016

vitorbal Aug 4, 2016

Choose a reason for hiding this comment

jrencz Aug 4, 2016

Choose a reason for hiding this comment

vitorbal commented Aug 4, 2016

eslintbot commented Aug 4, 2016

gyandeeps commented Aug 4, 2016

vitorbal Aug 5, 2016 • edited

Choose a reason for hiding this comment

ilyavolodin Aug 9, 2016

Choose a reason for hiding this comment

ilyavolodin commented Aug 9, 2016

gyandeeps commented Aug 9, 2016

vitorbal commented Aug 10, 2016

Update: make `max-lines` report the actual number of lines (fixes #6766) #6764

Update: make `max-lines` report the actual number of lines (fixes #6766) #6764

vitorbal Aug 5, 2016 •

edited