Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Guard against key-less nodes. Fixes #71 #72

Open
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

mwpastore
Copy link

This is very rough with no tests, intended mostly to further the discussion on #71 for now.

Copy link
Member

@trentmwillis trentmwillis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me. In the event the node doesn't have the property, it is obviously not the one we're interested in.

@rwjblue rwjblue requested a review from scalvert July 7, 2017 12:35
Copy link
Collaborator

@scalvert scalvert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me.

Copy link
Member

@trentmwillis trentmwillis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mwpastore do you think you can add a test or two for this? After that, we can land this!

@mwpastore
Copy link
Author

@trentmwillis Hrrm, how can I add a babel plugin to a single RuleTester run unit?

@trentmwillis
Copy link
Member

Good question, I don't know exactly. But, looking at the docs, looks like you can define it in the hash just like you would in the ESLint config.

Any additional properties of a test case will be passed directly to the linter as config options.

See here for more info.

nlfurniss and others added 16 commits November 29, 2017 00:18
ember-best-practices#93)

* Fixes require-ember-lifeline rule to scope it to only ember file types
* Add missing tests

This adds two failing tests to highlight bugs and a passing but
previously missing test.

The missing test added is for calling `set` which was destructured from
`import Ember from 'ember'`

One failing test is to handle the case where `Ember.computed` is called
from an import that isn't named `Ember`, eg
```
import E from 'ember';
E.computed()
```

Another failing test illustrates that ember import bindings are
clobbered if followed by any other default import, as in:
```
import Ember from 'ember';
import AnythingElse from 'anywhere';
```

* Clean up no-side-effect-cp rule.

Fixes a number of the errors previously identified, as well as a few
additional ones (tests added in this commit).
TypeError: Cannot read property 'getFilename' of undefined at
isLintableFile (/node_modules/eslint-plugin-ember-best-practices/lib/rules/require-ember-lifeline.js:33:53)
at
MemberExpression (/node_modules/eslint-plugin-ember-best-practices/lib/rules/require-ember-lifeline.js:69:14)
at listeners.(anonymous
function).forEach.listener (/node_modules/eslint/lib/util/safe-emitter.js:47:58)
at Array.forEach ()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants