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

Update: improve max-statements-per-line message (fixes #6287) #7044

Merged

Conversation

ljharb
Copy link
Sponsor Contributor

@ljharb ljharb commented Sep 2, 2016

What issue does this pull request address?
#6287

What changes did you make? (Give an overview)

  • Deferred reporting until after the line was finished parsing
  • Changed the "empty block" message
  • Added "count of statements per line" to normal message
  • Updated test cases to account for new messaging.
  • Followed @btmills' suggestions here

Is there anything you'd like reviewers to focus on?
Not in particular.

ljharb referenced this pull request in ljharb/eslint Sep 2, 2016
@@ -35,11 +35,12 @@ module.exports = {
const sourceCode = context.getSourceCode(),
options = context.options[0] || {},
maxStatementsPerLine = typeof options.max !== "undefined" ? options.max : 1,
message = "This line has too many statements. Maximum allowed is {{maxStatementsPerLine}}.",
data = { maxStatementsPerLine };
emptyBlockMessage = "This line has an empty block, but the maximum allowed is 0.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message doesn't seem to fit. It reads like the maximum number of empty blocks is zero, but it's actually the maximum number of statements. I'm actually a bit confused as to why this rule checks this condition at all since we have no-empty already.

@mysticatea any idea? It looks like you were the last to touch this.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment farther down in the file suggests it's for backwards compatibility.

You're right though that this message could be made clearer - it just didn't seem like "This line has N statements" made sense. Any suggestions?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Specifically, git blame points to 72335eb

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I wrote it in #6192 to not break this existing test.

@ljharb ljharb force-pushed the ljharb/max_statements_per_line_message branch from 798ca11 to 10cd56b Compare September 3, 2016 05:43
@eslintbot
Copy link

LGTM

@ljharb
Copy link
Sponsor Contributor Author

ljharb commented Sep 3, 2016

Pushed a slight tweak (and fixed commit authorship).

@ljharb ljharb force-pushed the ljharb/max_statements_per_line_message branch from 10cd56b to 03b7f37 Compare September 3, 2016 06:21
@eslintbot
Copy link

LGTM

@@ -35,11 +35,12 @@ module.exports = {
const sourceCode = context.getSourceCode(),
options = context.options[0] || {},
maxStatementsPerLine = typeof options.max !== "undefined" ? options.max : 1,
message = "This line has too many statements. Maximum allowed is {{maxStatementsPerLine}}.",
data = { maxStatementsPerLine };
emptyBlockMessage = "This line has an empty block, but no statements are allowed.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should not add a special message and rather remove the empty block check later (see #7051). If you can revert this piece, I think the rest is good to go.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would mean it says "there are 0 statements, the max is 0" - which makes no sense either. :-/ I think something special has to be done here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'm saying is that we will remove that special case completely so no one will see that. Nothing special needs to be done because it's going away.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, fair enough. Will update soon.

@ljharb ljharb force-pushed the ljharb/max_statements_per_line_message branch from 03b7f37 to 3a2d367 Compare September 4, 2016 00:04
@eslintbot
Copy link

LGTM

@@ -35,11 +35,11 @@ module.exports = {
const sourceCode = context.getSourceCode(),
options = context.options[0] || {},
maxStatementsPerLine = typeof options.max !== "undefined" ? options.max : 1,
message = "This line has too many statements. Maximum allowed is {{maxStatementsPerLine}}.",
data = { maxStatementsPerLine };
message = "This line has {{numberOfStatementsOnThisLine}} statement(s). Maximum allowed is {{maxStatementsPerLine}}.";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How hard would it be to correctly pluralize "statement(s)"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{{statements}} as placeholder; and in data, statements: maxStatementsPerLine === 1 ? "statement" : "statements".

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can do that. My thinking was that having the non-variable part be identical would make it easier for people doing log parsing. Should I make this change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't considered log parsing. However, my feeling is that if log parsing
were that important, the rule name is what should be parsed (and/or folks
can choose a formatter that is friendlier to automated parsing). To me, a
rule message should be regarded as human readable, not computer parsable,
and other fields should be used for parsing.

Let's wait for a second opinion though. 😄

On Sep 4, 2016 6:04 PM, "Jordan Harband" notifications@github.com wrote:

In lib/rules/max-statements-per-line.js
#7044 (comment):

@@ -35,11 +35,11 @@ module.exports = {
const sourceCode = context.getSourceCode(),
options = context.options[0] || {},
maxStatementsPerLine = typeof options.max !== "undefined" ? options.max : 1,

  •        message = "This line has too many statements. Maximum allowed is {{maxStatementsPerLine}}.",
    
  •        data = { maxStatementsPerLine };
    
  •        message = "This line has {{numberOfStatementsOnThisLine}} statement(s). Maximum allowed is {{maxStatementsPerLine}}.";
    

Sure, I can do that. My thinking was that having the non-variable part be
identical would make it easier for people doing log parsing. Should I make
this change?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/eslint/eslint/pull/7044/files/3a2d36721e241376db8c6e3ba89ded485706d976#r77460195,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AARWejS3tsGlv4px1lnK2kz6EdYh8RHzks5qm05-gaJpZM4J0Hsx
.

Copy link
Member

@btmills btmills Sep 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point, @ljharb. IIRC, the position in the past has been that those wanting to do computer consumption of the output should use a formatter intended for non-human consumption or use the API directly. Does that sound reasonable?

Edit: What @platinumazure said ⬆️

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I'll update to use the correct pluralization.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@ljharb ljharb force-pushed the ljharb/max_statements_per_line_message branch from 3a2d367 to cce29dd Compare September 5, 2016 01:39
@eslintbot
Copy link

LGTM

@platinumazure
Copy link
Member

LGTM! @btmills Perhaps you'd like to take another look?

@btmills
Copy link
Member

btmills commented Sep 6, 2016

LGTM, thanks @ljharb!

@btmills btmills merged commit ff19aa9 into eslint:master Sep 6, 2016
@ljharb ljharb deleted the ljharb/max_statements_per_line_message branch September 6, 2016 16: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

7 participants