Skip to content

Conversation

pascalduez
Copy link
Member

@valeriangalliat
Copy link
Member

Awesome work!

Feels really good to remove all these ignore comments.

@pascalduez
Copy link
Member Author

Plus it's driving out much more possible issues than JShint.

@valeriangalliat feel free to hack in if you see something.

@KittyGiraudel
Copy link
Member

Terrific. Only the tests to fix before merging. :)

@pascalduez
Copy link
Member Author

Something to discuss:

src/cli.js
  45:6  error  Don't use process.exit(); throw an error instead  no-process-exit
  48:4  error  Don't use process.exit(); throw an error instead  no-process-exit

http://eslint.org/docs/rules/no-process-exit.html

I guess we can mute this rule.

@KittyGiraudel
Copy link
Member

Any reason we do exit instead of throwing an error?

@valeriangalliat
Copy link
Member

Any reason we do exit instead of throwing an error?

To control the exit code.

+1 to mute the rule, it's not meaningful in the CLI, it's only relevant inside the library code (and we don't exit from the library).

@valeriangalliat
Copy link
Member

searchForMatches is calling the isAnnotatedByHand function passed as a parameter. It don't uses the upper scope declared function (though the calling code is actually passing it this very function, see searchForMatches calls).

@pascalduez
Copy link
Member Author

Yeah, I removed my comment, just realized the isAnnotatedByHand.bind(...)
Renaming the parameter into isAnnotatedByHandProxy

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.09% when pulling ab89646 on eslint into 81ec9e3 on develop.

@pascalduez pascalduez changed the title [WIP] Switch to ESlint [RFR] Switch to ESlint Mar 22, 2015
@pascalduez
Copy link
Member Author

So this is now Ready for review :-)

We are using all of ESlint default rules, overriding just the few ones we "don't agree" with.
There's also a couple of style rules added.
I personally like this approach.

Another option is not to use any base rules with --reset and set all the rules our self.

If you see something that should be added ?

@valeriangalliat
Copy link
Member

I like it as is, okay to merge. 👍

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.09% when pulling 1aaed23 on eslint into 81ec9e3 on develop.

@pascalduez
Copy link
Member Author

What about moving to adding a space between function declaration name and params.
Which brings us closer to standard

function whatever (foo, bar) { ... } 

@valeriangalliat
Copy link
Member

I'm all for following standard. I'm also okay with semistandard which is basically like standard but with semicolons (and thus will be more friendly with our codebase).

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.09% when pulling b8c4746 on eslint into 81ec9e3 on develop.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.09% when pulling b8c4746 on eslint into 81ec9e3 on develop.

pascalduez added a commit that referenced this pull request Apr 2, 2015
@pascalduez pascalduez merged commit 0d08922 into develop Apr 2, 2015
@pascalduez pascalduez deleted the eslint branch April 2, 2015 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants