Do not autofix self-referential computed properties in require-computed-macros rule

[runspired]

partially resolves #1118 (we should still add a new rule)

[bmish]

Instead of duplicating the context.report call, we should actually just return null inside the fixer function if we encounter a non-fixable situation. That goes for all these reporting functions and it will simplify all this code a lot.

fix(fixer) {
  if (!isFixable(...)) { return null; }
  ...  
}

[runspired]

@bmish I did that initially but it is linted against, I also suspect it's not good practice since it might cause eslint to report the violation as fixable when it is not.

[bmish]

It's not linted against, we actually have some examples of autofixers with return null in our codebase. And it's not bad practice, in fact it's common practice to have autofixers which ignore certain situations, and this is how we do it. I can assure you it will result in much simpler code :)

[runspired]

this was the lint rule it triggered https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/master/docs/rules/fixer-return.md

[bmish]

Using return null won't trigger that rule. return null indicates that you don't want to autofix in a particular situation.

[bmish]

We should use output: null to indicate there's no autofix, it's more clear and concise.

[runspired]

wouldn't that imply the rule was deleting code?

[bmish]

No. It means there's no autofix.

And there's actually a lint rule to enforce this but unfortunately it's not working on this code because the array of test cases has .map(addComputedImport) on it.

https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/master/docs/rules/prefer-output-null.md

[bmish]

I think we're adding a few redundant test cases as well as missing some test cases.

We need the following test cases basic test cases which test a simple single-arg return expression that will be converted into the reads macro:

• class Test { @computed() get foo() { return this.foo; } }
• class Test { foo = computed(function() { return this.foo; }) } // non-decorator style
• import EmberObject from '@ember/object'; EmberObject.extend({ foo: computed(function() { return this.foo; }) } // classic class

And we need to test other types of expressions that will be converted into other macros:

• class Test { @computed() get a() { return this.a && this.b; } } // logical
  *class Test { @computed() get a() { return this.a > this.b; } } // binary
  *import EmberObject from '@ember/object'; EmberObject.extend({ computed(function() { return this.chores.filterBy('done', true); }) // function

We don't need to test anything related to the dependent key arguments (computed('this-dependent-key-does-not-matter', function () {})) since those end up ignored anyway.

[runspired]

The redundancy is intentional, it covers both forms of classic and both forms of class based declarations both with and without args being supplied to the computed as dependent keys (since I encountered errors from conversion from both in the wild).

Can add tests for a few of the others.

[bmish]

Sounds good. A comment next to each test case explaining what variation it is testing would help.

[bmish]

There should always be a parent, but we do need to check what node type the parent is before we check its key. That way, we won't end up using the key of some unexpected kind of node.

[runspired]

oddly I believe I encountered a no parent situation in our own test suite. Will double check that.

[bmish]

ESLint adds the parent when it runs the rule test cases. But if you write separate unit tests for this function by itself, then it won't have a parent. So feel free to add the parent guard but you only need if you write separate unit tests for the function itself.