Add fix for use-ember-get-and-set

[sudowork]

Continuation of #55. This allows this.get() and this.set() in the tests directory, and it will completely skip over the mirage directory.

[sudowork]

@SaladFork @michalsnik @tylerturdenpants Here's the PR continuing from #55. Since I don't have write access to the original PR, I opened a new one with the commits rebased.

[kevinkucharczyk]

Maybe we should make this configurable, so that a user can provide options on which directories they want to skip?

[sudowork]

Makes sense to me. I was considering this, but I didn't have a great way to specify the option. I was thinking either:

• Only allow them to specify a single part of the path as we do here.
• Substring match. Not great and could lead to false positives.
• Substring match but make sure that we're respecting path boundaries (e.g. app/nottests would not match to tests.
• Regex match. Could be a little heavy handed.
• Glob matching. Would probably require requiring in another dependency.

Alternatively, the users could include a .eslintrc file in the subdirectory they want to exclude.

// some/subdirectory/to/exclude/.eslintrc
{
  "ember/use-ember-get-and-set": "off"
}

[kevinkucharczyk]

Right, a .eslintrc file does seem like the easiest approach here. So I think we can leave out the option for now and rely on users excluding directories on their own, I like this :)

[kevinkucharczyk]

We're still a little worried about the fix potentially breaking a build. When you run the fix and you don't have set/get imported, your app won't build anymore.
[https://github.com//pull/55#issuecomment-299720671](A suggestion) was to replace this.set with Ember.set etc., to only trigger a different lint error instead of breaking the app.
However, that might not work in all cases: with the new module imports, we would be able to, for example, import Component from '@ember/component'; without ever needing to import Ember itself. Thus, replacing this with Ember would still break builds.
In the end, if we don't come up with a solution for this, maybe it would still be okay to get this merged but supply a warning that this fix might potentially break your app.

[sudowork]

Definitely a valid concern.

Here are a few enhancements I think I could make:

• Don't autofix (still report) if Ember is not imported. This would reduce what is fixable to avoid the import issue you described.
• Check if the method under question is declared in an upper scope. If not, then use Ember.get instead of get for example.

What are your thoughts?

[sudowork]

@kevinkucharczyk I updated the PR with some new behaviors to prevent code breakages:

• Don't attempt to fix the code if Ember is not imported
• When fixing the code, avoid breakages by using the local module if present. Otherwise, use the Ember.get form.
  • A new traversal over VariableDeclarators in the module scope is used to collect any local modules that were destructured. This only checks for the object destructuring form of const { get } = Ember;. It is also alias-aware: const { get: emberGet } = Ember; this.get('foo'); => const { get: emberGet } = Ember; emberGet(this, 'foo');. It will not detect const get = Ember.get.
  • During fixing, if no local module is present, then default to using Ember.get (also alias aware).

[kevinkucharczyk]

Looks pretty good! One more thing though: I think it would be good to document the new behaviour in use-ember-get-and-set.md, so that users won't have to read through the source code in order to understand what the rule does and how the fix works.

[sudowork]

Documented the --fix behavior.

[kevinkucharczyk]

Please also add a note saying that the linter skips mirage and tests :)

[sudowork]

Done!

[michalsnik]

Very good work, thank you so much @sudowork !