Copy over fixtures from ember-suave

[RobbieTheWagner]

@brzpegasus this is a WIP, copied over all the fixtures blindly from ember-suave, and we have several failures. I'll work on resolving those failures now.

[RobbieTheWagner]

@alexlafroscia @brzpegasus it seems that the ember-suave rule requires no spaces in generators before the * and can either have a space or no space after it. Is this correct? Could we decide on space or no space after *? ESLint cannot do an optional space after the star.

[brzpegasus]

I believe I left the space after * unspecified because we want to allow the following:

// good
function* foo() {} // space required after *
let bar = function*() {} // no space after *, for consistency with normal anonymous functions

The anonymous function restriction is technically enforced by the rule that forbids spaces before parentheses for anonymous functions:

// good
let foo = function() {}

// bad
let bar = function () {}

[RobbieTheWagner]

@brzpegasus yeah, I agree with the way rules were set up. It seems ESLint won't allow an optional space after * though. It's either require no space or require a space. I posted an issue to change the rule, hopefully.

[RobbieTheWagner]

What about catch() vs catch ()? Seems that space after keyword and space before keyword good/bad files conflict. Not sure how tests passed before

[RobbieTheWagner]

@brzpegasus correct me if I am wrong, but the fixtures good/bad examples should adhere to all other rules, except the one we are testing right?

[brzpegasus]

What about catch() vs catch ()? Seems that space after keyword and space before keyword good/bad files conflict. Not sure how tests passed before

Looks like we never had a rule for it 😛. DockYarders favor catch (). Some of the "good" fixtures might have to be updated.

[brzpegasus]

the fixtures good/bad examples should adhere to all other rules, except the one we are testing right?

Confirm.

[brzpegasus]

DockYarders favor catch ()

I take it back. Should be catch(), similar to function() {}.

[RobbieTheWagner]

@brzpegasus one of the big things that seemed to be wrong in all the examples was array spacing. Are we supposed to enforce it or not?

I've got it down to just 4 errors now, one of which is a flaw with ESLint and not sure why the other 3 do not work.

Please take a look at what I have done and let me know if anything seems off.

[RobbieTheWagner]

Seems like, for some reason, newline-after-var only works for vars and not let or const, but the rule docs say it should work for everything.

[RobbieTheWagner]

@brzpegasus @alexlafroscia have either of you had that issue with newline-after-var before?

[brzpegasus]

In fixtures/const-usage/good/example.js, I had to temporarily comment out a const statement because there was no new line after it per the newline-after-var rule, so it is definitely recognizing const.

[brzpegasus]

one of the big things that seemed to be wrong in all the examples was array spacing. Are we supposed to enforce it or not?

If by "wrong", you mean that arrays are formatted as ['a', 'b', 'c'] instead of [ 'a', 'b', 'c' ], then no, that is intentional. We discussed it internally here.

[RobbieTheWagner]

@brzpegasus it doesn't seem to work for the const.js and let.js newline rule ones though :(. As for array spacing, someone enforced array spacing in the config, so I assumed it was supposed to be done. I will change it to enforce no space in arrays instead.

[RobbieTheWagner]

Okay, I fixed the array spacing to be the correct one. @brzpegasus you have any idea why require-line-break-after-variable-assignment/bad/const.js and require-line-break-after-variable-assignment/bad/let.js are not failing like they should? It seems they work, if I change them to var instead.

[brzpegasus]

Okay, I fixed the array spacing to be the correct one.

Thanks! I didn't realize we had that many files with the incorrect array spacing.

you have any idea why require-line-break-after-variable-assignment/bad/const.js and require-line-break-after-variable-assignment/bad/let.js are not failing like they should?

I will check.

[RobbieTheWagner]

@brzpegasus you didn't have any incorrect array spacing files. The problem was this plugin initially had the wrong array spacing rule defined, and I assumed all the rules were correct when I started.

[brzpegasus]

Ah, gotcha.

For const.js and let.js, which rule is supposed to disallow let abc = 1; let foo = 2;? I don't believe newline-after-var is intended to address that scenario. It only checks whether you have an empty line after the last variable declaration.

I think when you change const and let to var in those files, the tests report that those files are failing due to the no-var rule, not because it somehow works with var. Not sure that there's an equivalent in ESLint, but we might be able to achieve the same results with max-statements-per-line.

[brzpegasus]

The problem was this plugin initially had the wrong array spacing rule defined

I blame polyjuice.

[RobbieTheWagner]

Ah, good catch. I will try that now

[RobbieTheWagner]

@brzpegasus max-statements-per-line did the trick! 😄 . Now we just have 3 issues. I believe all of them are known limitations with ESLint, so not sure we can do any further, without writing some more custom rules or proposing changes to ESLint.

I proposed the change to ESLint for the generator issue here: https://github.com/eslint/eslint/issues/6777

[RobbieTheWagner]

@brzpegasus since this PR gets us most of the way there, what would you think about merging it and opening separate issues for the remaining 3, since they will take some work?

[RobbieTheWagner]

@brzpegasus how are we looking? Let me know anything else that needs to be addressed.

[brzpegasus]

I think the only thing left to adjust is the config to not allow spaces after catch.

[RobbieTheWagner]

@brzpegasus just made that change. Let me know if anything else needs to be done

[brzpegasus]

:+1: Thank you!

[brzpegasus]

I can't merge PRs that have failing tests from a phone (didn't know that was a thing!). I'll get to it later from my laptop.

[RobbieTheWagner]

Sounds good, thanks!

[RobbieTheWagner]

@brzpegasus once you merge this in, can you also bump the version and release to npm? Would love to have this out and useable while we wrangle the last few issues.

[RobbieTheWagner]

Thanks for the merge @brzpegasus!