Fix: object-shorthand's consistent-as-needed option (issue #7214) #7215
Conversation
|
@naomiajacobs, thanks for your PR! By analyzing the annotation information on this pull request, we identified @martijndeh, @vitorbal and @NickHeiner to be potential reviewers |
|
LGTM |
| @@ -156,7 +156,7 @@ module.exports = { | |||
| property.value && property.value.id && property.value.id.name === property.key.name || | |||
|
|
|||
| // A property | |||
| property.value && property.value.name === property.key.name | |||
| property.value && (property.value.name || property.value.value) === (property.key.name || property.key.value) | |||
There was a problem hiding this comment.
This comparison seems still wrong about computed properties and string literals.
Could you add tests such below?
({foo: "foo"})({"❤": "❤"})({[foo]: foo})({[foo]: "foo"})
I guess isRedudant function should be:
const astUtils = require("../ast-utils");
function isRedudant(property) {
const value = property.value;
if (value.type === "FunctionExpression") {
return !value.id; // Only anonymous should be shorthand method.
}
if (value.type === "Identifier") {
return astUtils.getStaticPropertyName(property) === value.name;
}
return false;
}There was a problem hiding this comment.
@mysticatea I will look into this - thanks for the feedback!
|
LGTM |
|
LGTM |
|
@mysticatea the requested changes have been added! However, I still have the CLA:error tag. I went and signed the CLA and pushed an empty commit to re-trigger the check, but it doesn't seem to have worked. Do you know how I can fix this? Thanks! |
|
@naomiajacobs Thank you! The 1st commit and the 2nd commit seem to have different author info. Could you sign CLA for both author info? |
| @@ -150,14 +155,16 @@ module.exports = { | |||
| * @private | |||
| **/ | |||
| function isRedudant(property) { | |||
There was a problem hiding this comment.
Should be isRedundant (might as well fix this while we're in here).
|
LGTM |
|
@vitorbal @platinumazure the typo was fixed - is that the bug the label is referring to? If it's fixed now, what's the process for getting it merged in? Let me know if I can help. Thanks! |
|
@naomiajacobs Nah, we use "bug" to indicate that this is a bug issue/PR, rather than an enhancement. @vitorbal Please confirm or clarify, thanks |
|
That's correct, I was just marking this accordingly with the corresponding issue! Sorry for any confusion. |
|
@vitorbal gotcha, makes sense. Do you know when this might get merged in? I ask because we'd love to be able to use this rule in our code base! Thanks! |
|
@naomiajacobs The latest this will be released is next Friday, 7 October. Our release schedule is every 2 weeks on Friday (or occasionally Saturday). So hopefully not later than that, assuming we don't let this sit for too long. I don't know if there's any chance this could make it into a patch release sooner than that. We usually only do patch releases for really urgent things, such as regressions from the last release. So normally we would want this fixed in a normal release. But we'll see what happens. |
|
Nice job, @naomiajacobs, and thanks for working on this! |
|
@platinumazure thanks for the clarification! @ahuth no problem :) |
eslint-deprecated
bot
added
the
archived due to age
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
This PR fixes issue #7214.
Please check each item to ensure your pull request is ready:
What changes did you make? (Give an overview)
object-shorthand'sconsistent-as-neededrule was failing when keys were either numbers or quoted strings. We (@ahuth and I) fixed this by changing the conditions under which an object is determined to have redundant properties.Is there anything you'd like reviewers to focus on?