clutchski / coffeelint

Lint your CoffeeScript.
http://www.coffeelint.org
Other
1.18k stars 171 forks source link

Allow no spaces around = for default arguments despite space_operators #419

Open mitar opened 9 years ago

mitar commented 9 years ago

I have space_operators enabled, but I would prefer no spaces for default arguments in function definitions:

foo = (bar=true) ->

This triggers an error. Could this be optional?

AsaAyers commented 9 years ago

Sounds like a reasonable option to me. It should probably be an option on space_operators maybe named default_parameters. It should default to the current behavior.

eddiemonge commented 9 years ago

That just seems inconsistent to me. Why would you have it one way somewhere and another elsewhere? Or you could make it universal inside parens to cover things like if (name=foo) and name isnt bar

mitar commented 9 years ago

Maybe I am just used to PEP8. :-)

Don't use spaces around the = sign when used to indicate a keyword argument or a default parameter value.

And about operators:

Always surround these binary operators with a single space on either side: assignment ( = ), augmented assignment ( += , -= etc.), comparisons ( == , < , > , != , <> , <= , >= , in , not in , is , is not ), Booleans ( and , or , not ).

So, I just like PEP8 so I am trying to do similar with my CoffeeScript code.

mnquintana commented 9 years ago

:+1: We'd need an option like this to be able to use space_operators in Atom. (https://github.com/atom/atom/pull/6898#discussion_r30905259)

mitar commented 9 years ago

@mnquintana: I just don't know how to implement this. At the lexer level it is too complicated (you do not know what = means), at the AST level you do not know the text around the token, were there spaces or not.

AsaAyers commented 9 years ago

Maybe we can use PARAM_START and PARAM_END to detect them. I can't think of any other legitimate reason an = might show up between those two.

I say legitimate because I think I'm pretty good at coming up with crazy examples. If someone does this... they need to stop.

crazy = false
foo = (foo = (crazy = true)) ->

console.log('crazy', crazy) # false

foo('hello')
console.log('crazy', crazy) # false

foo()
console.log('crazy', crazy) # true
mitar commented 9 years ago

Yea, I have them in the test case. :-)

mitar commented 9 years ago

So AST does not retain the information about the tokens? So that we could go back and see what if the = has space before and after it? Or not? We could do token -> location in string -> get substring -> check for space before and after?

AsaAyers commented 9 years ago

In my opinion we should go go the simple route and if anyone can show up with a convincing argument why foo = (bar=(a=1)) -> is legitimate then we can see about fixing that edge case.

CoffeeLint helps find code that while it technically compiles is bad (for some definition of bad). I'm totally fine with these kinds of "bugs" revealing bad patterns in people's code. That's assuming anyone is writing code like we see below and are using coffeelint.

foo = (bar=(a=1)) ->
    ^ needs a space because its an operator
foo = (bar=(a=1)) ->
          ^ no space because it's a default parameter
foo = (bar=(a=1)) ->
             ^ Should need a space but I don't care if this doesn't work
mitar commented 9 years ago

See #438 for the implementation.