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
Update: improve max-statements-per-line
message (fixes #6287)
#7044
Conversation
@@ -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.", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
798ca11
to
10cd56b
Compare
LGTM |
Pushed a slight tweak (and fixed commit authorship). |
10cd56b
to
03b7f37
Compare
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.", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
03b7f37
to
3a2d367
Compare
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}}."; |
There was a problem hiding this comment.
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)"?
There was a problem hiding this comment.
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"
.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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 ⬆️
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
3a2d367
to
cce29dd
Compare
LGTM |
LGTM! @btmills Perhaps you'd like to take another look? |
LGTM, thanks @ljharb! |
What issue does this pull request address?
#6287
What changes did you make? (Give an overview)
Is there anything you'd like reviewers to focus on?
Not in particular.