New rule: no-duplicate-dependent-keys #104
Conversation
|
It just occurred to me during this migration of rules from our old repo to this one - we're losing attribution of the original author during this process. We could do one of two things: Provide attribution via distinguishing between the author and committer Thoughts, @jbandura @michalsnik? |
|
I agree, but this rule is not a migration :) I added my thoughts in #103. |
|
Ah, you're right. Thanks! |
lib/utils/utils.js
Outdated
| * @param {CallExpression} callExp Given call expression | ||
| * @return {Boolean} Flag whether dependent keys present. | ||
| */ | ||
| function hasDuplicateDependentKeys(callExp) { |
There was a problem hiding this comment.
This function should be in ember.js util. We want these utils to be independent from ember-related stuff.
lib/utils/utils.js
Outdated
| * @param {CallExpression} callExp CallExpression to examine | ||
| * @return {Array} Array of unwrapped dependent keys | ||
| */ | ||
| function parseDependentKeys(callExp) { |
lib/utils/utils.js
Outdated
| * @param {Array} dependentKeys array of strings containing unprocessed dependent keys. | ||
| * @return {Array} Array of unwrapped dependent keys | ||
| */ | ||
| function unwrapBraceExpressions(dependentKeys) { |
lib/utils/utils.js
Outdated
| * @return {Array} Array of unwrapped dependent keys | ||
| */ | ||
| function parseDependentKeys(callExp) { | ||
| const isMemberExpCallExp = callExp.arguments.length === 0 && callExp.callee.object; |
There was a problem hiding this comment.
I'd add comment above why do we check this. I was confused at first and then I saw a case with .volatile in tests.
You should also check it rather like so:
!callExp.arguments.length &&
isMemberExpression(callExp.callee) &&
isCallExpression(callExp.callee.object)
lib/utils/utils.js
Outdated
| const dependentKeys = args | ||
| .filter(arg => isLiteral(arg)) | ||
| .map(literal => literal.value); | ||
| return unwrapBraceExpressions(dependentKeys); |
There was a problem hiding this comment.
Add empty line before return
There was a problem hiding this comment.
good point, we should probably configure it in the project's eslint I think
lib/utils/utils.js
Outdated
| return properties.map(property => `${prefix}.${property}`); | ||
| }); | ||
|
|
||
| return unwrappedExpressions |
There was a problem hiding this comment.
What if you have dependent key like: lorem.ipsum.{dolor,sit}? I think this function will return:
[
'lorem.ipsum'
]
There was a problem hiding this comment.
on the other hand, this will not work in Ember, as only one level of nesting is supported, WDYT @michalsnik?
There was a problem hiding this comment.
This works actually @jbandura, only multi-level braces nesting is not allowed. See:
https://ember-twiddle.com/57f5af38632a6770fcae0c0904c07e08?openFiles=controllers.application.js%2C
| { | ||
| code: ` | ||
| { | ||
| foo: computed('model.foo', 'model.bar', 'model.baz', function() {}) |
There was a problem hiding this comment.
We should test more cases, those with @each and [] too. People tend to create really crazy dependent keys and we need to make sure we support all of those weird cases, so I recommend to enlarge the test suite for this rule significantly :) WDYT?
There was a problem hiding this comment.
makes sense, will do 👍
Add support for unwrapping keys with .@each and .[] along with multiple nesting
|
fleshed out the test suite + added support for more complex dependent keys, let me know what you think @michalsnik :) |
…slint-plugin-ember into 59-no-duplicate-dependent-keys
jbandura
force-pushed
the
59-no-duplicate-dependent-keys
branch
from
eeb011c
to
dc4b57f
Compare
|
Looks like we have some conflicts, please take care of them :) |
|
@michalsnik I'd prefer it if we could rebase this branch on |
|
@Turbo87 I'm squashing PRs while merging anyway, so master's history will not be disrupted :) |
|
@michalsnik I'm not a huge fan of squashing commits since it makes |
|
@Turbo87 I get it, on the other hand squased PRs result in much cleaner history. I don't remember the need of digging into single commits inside PRs - those are usually not so big anyway and squashed are well-contained and easier to grasp. Knowing which PR broke something was more than enough in my case. Single commits inside PRs are often not too descriptive and contain bunch of bugs resolved by further commits inside the same PR.. |
| docs: { | ||
| description: 'Disallow repeating dependent keys', | ||
| category: 'General', | ||
| recommended: true |
There was a problem hiding this comment.
Please make it not recommended, so we can do a minor release first. I didn't catch this earlier
Resolves #59.
Introducing new rule
no-duplicate-dependent-keys.