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

Dots at the end of many messages are omitted #6762

Closed
markelog opened this issue Jul 26, 2016 · 21 comments
Closed

Dots at the end of many messages are omitted #6762

markelog opened this issue Jul 26, 2016 · 21 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion good first issue Good for people who haven't worked on ESLint before needs bikeshedding Minor details about this change need to be discussed rule Relates to ESLint's core rules

Comments

@markelog
Copy link
Member

What version of ESLint are you using?
3.1.1
What parser (default, Babel-ESLint, etc.) are you using?
default

Please show your full configuration:
Example for array-bracket-spacing but this issue is not limited to this rule only -

{
    "rules": {
        "array-bracket-spacing": "error"
    }
}

What did you do? Please include the actual source code causing the issue.

var foo = [ ]

What did you expect to happen?
Have a dot at the end -
1:11 error There should be no space after '['. array-bracket-spacing

What actually happened? Please include the actual, raw output from ESLint.
It doesn't have a dot at the end -
1:11 error There should be no space after '[' array-bracket-spacing

I'm not sure if this is intentional or not, every message should have a dot at the end or there is a different policy in place? If so, I think it should be documented.

Beside that, if this is really a "bug" I think every rule should be fixed with one commit in order not to trash the commit history

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Jul 26, 2016
@markelog markelog added rule Relates to ESLint's core rules needs bikeshedding Minor details about this change need to be discussed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion bug ESLint is working incorrectly and removed triage An ESLint team member will look at this issue soon labels Jul 26, 2016
@platinumazure
Copy link
Member

As I noted in another issue, be careful to check this with a formatter that isn't "stylish", which does strip periods from all messages. (The ESLint demo is a sufficient tool for checking the error messages, as well as most if not all of the other formatters.)

@markelog
Copy link
Member Author

What do you mean? Can't we just check the source? If we want to act on it, then I would prefer to add beginner label, this issue seems perfect for the first timer

@markelog
Copy link
Member Author

markelog commented Jul 26, 2016

Looking it from the other perspective we can just remove the dots from all the rules and re-add them with reporter, this would be an opposite of the stylish logic.

That way, we wouldn't need to "remember" check those dots when reviewing new PRs

@markelog
Copy link
Member Author

That way, we wouldn't need to "remember" check those dots when reviewing new PRs

With this path though, we would need to check for absence of those dots :), but I guess reporters could check for their presence, but that wouldn't work for custom reporters then, doesn't sound like a perfect solution

@pmcelhaney
Copy link
Contributor

How about updating the reporter to add the dot if and only if it's missing? That way 3rd party rules would also be normalized. (Not sure if that's a big or a feature. :))

@markelog
Copy link
Member Author

@pmcelhaney it still be inconsistent in the sources though.

btw, @wavebeem do you want to get in on this?

@markelog markelog added the good first issue Good for people who haven't worked on ESLint before label Jul 28, 2016
@pmcelhaney
Copy link
Contributor

Yeah.

catching manually in PR < fixing at runtime < catching at build time < catching and fixing with a custom ESLint rule

Just not sure what's possible at this point.

@pmcelhaney
Copy link
Contributor

pmcelhaney commented Jul 28, 2016

Okay, maybe a check for proper formatting can be added to rule-tester.js.

https://github.com/eslint/eslint/blob/master/lib/testers/rule-tester.js#L407

Is there a way to add that check only to this repo but not affect third-party rules that doesn't affect the spirit of rules being independent?

@wavebeem
Copy link
Contributor

Hmm, normalizing the presence/absence of . at the end of messages in code sounds pretty smart to me, especially considering custom rules are a core feature of ESLint. I dunno.

@vitorbal
Copy link
Member

We could add a custom internal rule for checking that the last dot is
always present, similar to what we did for enforcing the correct structure
of the "meta" property in rule definitions.
I just wonder if that wouldn't be overkill.
On Fri, Jul 29, 2016 at 12:53 AM Brian Mock notifications@github.com
wrote:

Hmm, normalizing the presence/absence of . at the end of messages in code
sounds pretty smart to me, especially considering custom rules are a core
feature of ESLint. I dunno.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#6762 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAmNdouQcUkvwoIFzPx2FY7BV5934DG9ks5qaTLugaJpZM4JVQy9
.

@pmcelhaney
Copy link
Contributor

pmcelhaney commented Jul 29, 2016

I messed around with adding another assert at line 395 of rule-tester.js.

                    if (messages[i].message) {
                        assert.equal(messages[i].message.slice(-1), ".", messages[i].message);
                    }

And found the following messages. If there's an interest, I may go through and normalize all of the messages in the source code this weekend.

Messages that do not end in "."

'A' is already defined
'A' was used before it was defined
'Object' is already defined
'a' is already defined
'a' was used before it was defined
'b' is already defined
'top' is already defined
A space is required after '['
A space is required after '{'
A space is required before ']'
A space is required before '}'
Avoid using Function.prototype.apply, instead use Reflect.apply
Avoid using Function.prototype.call, instead use Reflect.apply
Avoid using Object.defineProperty, instead use Reflect.defineProperty
Avoid using Object.getOwnPropertyDescriptor, instead use Reflect.getOwnPropertyDescriptor
Avoid using Object.getOwnPropertyNames, instead use Reflect.getOwnPropertyNames
Avoid using Object.getPrototypeOf, instead use Reflect.getPrototypeOf
Avoid using Object.isExtensible, instead use Reflect.isExtensible
Avoid using Object.preventExtensions, instead use Reflect.preventExtensions
Avoid using Object.setPrototypeOf, instead use Reflect.setPrototypeOf
Avoid using the delete keyword, instead use Reflect.deleteProperty
Do not nest ternary expressions
Don't make functions within a loop
Expected space or tab before '*/' in comment
Expected whitespace after rest operator
Expected whitespace after rest property operator
Expected whitespace after spread operator
Expected whitespace after spread property operator
File must be at most 2 lines long
Getter is not present
Identifier 'arr' is blacklisted
Identifier 'bar' is blacklisted
Identifier 'baz' is blacklisted
Identifier 'foo' is blacklisted
Identifier 'key' is blacklisted
Identifier 'myArray' is blacklisted
Identifier 'myDate' is blacklisted
Identifier 'obj' is blacklisted
Identifier name '_$x$_t$' is too long. (> 4)
Identifier name '_$xt_$' is too long. (> 4)
Identifier name 'a' is too short. (< 2)
Identifier name 'e' is too short. (< 2)
Identifier name 'i' is too short. (< 2)
Identifier name 'j' is too short. (< 2)
Identifier name 'x' is too short. (< 2)
Invalid flags supplied to RegExp constructor 'u'
Invalid flags supplied to RegExp constructor 'y'
Invalid flags supplied to RegExp constructor 'yu'
Invalid flags supplied to RegExp constructor 'z'
Invalid regular expression: /)/: Unmatched ')'
Invalid regular expression: /[/: Unterminated character class
Invalid typeof comparison value
Irregular whitespace not allowed
Missing '()' invoking a constructor
Missing space before =>
No magic number: -60
No magic number: 0
No magic number: 0x1A
No magic number: 1
No magic number: 10
No magic number: 2
No magic number: 3
No magic number: 4
No magic number: 42
No magic number: 5
No magic number: 60
Number constants declarations must use 'const'
Object properties must go on a new line
Object properties must go on a new line if they aren't all on the same line
Setter is not present
The 'in' expression's left operand is negated
There should be no line break before or after '+'
There should be no line break before or after '+='
There should be no line break before or after '='
There should be no line break before or after '?'
There should be no line break before or after '||'
There should be no space after '['
There should be no space after '{'
There should be no space before ']'
There should be no space before '}'
Unexpected parentheses around single function argument
Unexpected space after =>
Unexpected space before =>
Unexpected space or tab before '*/' in comment
Unexpected use of 'event'
Unexpected use of 'foo'
Unexpected use of continue statement
Unexpected whitespace after rest operator
Unexpected whitespace after rest property operator
Unexpected whitespace after spread operator
Unexpected whitespace after spread property operator
Unnecessary escape character: \"
Unnecessary escape character: \#
Unnecessary escape character: \$
Unnecessary escape character: \'
Unnecessary escape character: \;
Unnecessary escape character: \@
Unnecessary escape character: \B
Unnecessary escape character: \a
Unnecessary escape character: \d
Unnecessary escape character: \p
Unnecessary use of boolean literals in conditional expression
Unnecessary use of conditional expression for default assignment
Unsafe usage of BreakStatement
Unsafe usage of ContinueStatement
Unsafe usage of ReturnStatement
Unsafe usage of ThrowStatement
Variables within the same declaration block should be sorted alphabetically

@markelog
Copy link
Member Author

markelog commented Jul 29, 2016

@vitorbal

We could add a custom internal rule for checking that the last dot is
always present

You mean custom ESLint rule? I'm not sure if that is possible for static analysis tool, for example, you would need to deal with cases like that -

message: "There should be no space after '" + token.value + "'",

@pmcelhaney

Okay, maybe a check for proper formatting can be added to rule-tester.js.

I think this is our best bet, it shouldn't reflect on rules which are not part of our repo, so we wouldn't need to bump major or anything like that

@pmcelhaney
Copy link
Contributor

@markelog The change to rule-tester.js is simple, but how do we prevent it from affecting third party rules? Don't they use rule-tester.js in their tests?

I was thinking maybe add a flag to package.json, so that it only affects the ESLint repo, but plugins can opt in to enforcing the strict message style if they want.

@markelog
Copy link
Member Author

markelog commented Jul 29, 2016

Wow, you right indeed, RuleTester is actually public interface.

I was thinking maybe add a flag to package.json

Seems a bit radical.

Or we can add additional option to it signature like strict: true or checkMessageDots or something, doesn't seem to be clean solution though since we would need to add it everywhere or make this behaviour by default but wait for major?

@pmcelhaney
Copy link
Contributor

Yeah, a flag that would have to be set every time RuleTester is constructed would defeat the purpose. So maybe add a property to the constructor itself?

   RuleTester.isValidMessageFormat = function (message) {
       return message.slice(-1) === ".";
   }

@ilyavolodin
Copy link
Member

I know that keeping all of the error messages in the same style is important, but is it important enough to add something like that to RuleTester, or create additional complexity in some other way?
I think we should just go through and add dots at the end of every message. Sometime in the future we can just do another pass. It's not like it takes all that much time.

@markelog
Copy link
Member Author

but is it important enough to add something like that to RuleTester, or create additional complexity in some other way?

Maybe not important per se, but I think it would be a good thing, from my perspective though not at this point of time, I think in scope of this ticket we can just add periods and probably continue this discussion at 4.0 time, sounds good?

@ilyavolodin
Copy link
Member

That sounds like a good plan to me.

@markelog
Copy link
Member Author

Okay, cool.

@pmcelhaney, @wavebeem would any of you want to help with this one?

@pmcelhaney
Copy link
Contributor

Sure, I'll do it this weekend.

@wavebeem
Copy link
Contributor

Be my guest; my weekend is looking a little busy. Thanks.

On Fri, Jul 29, 2016 at 2:15 PM, Patrick McElhaney <notifications@github.com

wrote:

Sure, I'll do it this weekend.


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

@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 bug ESLint is working incorrectly evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion good first issue Good for people who haven't worked on ESLint before needs bikeshedding Minor details about this change need to be discussed rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

7 participants