Fix: no-useless-rename handles ExperimentalRestProperty (fixes #6284) #6288

platinumazure · 2016-05-30T19:31:40Z

I had a couple of options here-- either compare the type explicitly, or just look for the absence of a key property. I went with the former to be safe, but I can certainly amend to the latter if people think that is better.

At this point I've only handled destructuring-- not sure yet if this is an issue in import or export. I'll investigate that and update this PR if needed. (EDIT: Seems those use a * token instead. In addition, that seems to be standard ES2015. So I assume we're good-- speak up if I missed something.)

…#6284)

eslintbot · 2016-05-30T19:31:41Z

LGTM

mention-bot · 2016-05-30T19:31:41Z

By analyzing the blame information on this pull request, we identified @kaicataldo and @alberto to be potential reviewers

kaicataldo · 2016-05-30T20:33:58Z

tests/lib/rules/no-useless-rename.js

@@ -239,6 +260,30 @@ ruleTester.run("no-useless-rename", rule, {
            errors: ["Destructuring assignment foo unnecessarily renamed.", "Destructuring assignment bar unnecessarily renamed."]
        },
        {
+            code: "const {foo: foo, ...stuff} = myObject;",
+            parserOptions: {


We should check the fixed output here as well - check the other invalid tests for the output key to see what this looks like

My bad, I didn't realize the rule was fixable. I will add that in a few
hours, when I get back home. Thanks!
On May 30, 2016 3:34 PM, "Kai Cataldo" notifications@github.com wrote:

In tests/lib/rules/no-useless-rename.js
#6288 (comment):

@@ -239,6 +260,30 @@ ruleTester.run("no-useless-rename", rule, {
errors: ["Destructuring assignment foo unnecessarily renamed.", "Destructuring assignment bar unnecessarily renamed."]
},
{

code: "const {foo: foo, ...stuff} = myObject;",

parserOptions: {

We should check the fixed output here as well - check the other invalid
tests for the output key to see what this looks like

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/eslint/eslint/pull/6288/files/80d4c3c664cd314b393a4b31e2878a704683645a#r65104582,
or mute the thread
https://github.com/notifications/unsubscribe/AARWeoNObAlcbU4o3J-4iSuxw4-CESFLks5qG0m3gaJpZM4Ip_rq
.

kaicataldo · 2016-05-30T20:36:22Z

One quick comment - thanks for working on this!

mysticatea · 2016-05-30T22:23:01Z

I'm sorry, I had forgotten this option at that time.
Thank you.

ilyavolodin · 2016-05-30T23:39:03Z

Hmm.. I would rather check for absence of the key, then use experimentalRestProperty, since as the name implies, it's experimental and will change at some point in the future. Which will mean we'll have to modify this rule.

kaicataldo · 2016-05-30T23:42:30Z

To tag on to what @ilyavolodin suggested, I think I'd like to see a comment explaining why the change is there then (since it won't be explicitly guarding against experimentalRestProperty).

platinumazure · 2016-05-30T23:57:46Z

In all honesty, I'm not sure what's best here. It doesn't help that astexplorer.net (using its babel-eslint setting) shows a SpreadProperty which has a key and value and wouldn't have caused the rule to throw. I'm not even really sure what the spec is honestly supposed to look like here.

Until we get consensus on the approach to use in the rule itself, I will focus on adding output to my test cases.

kaicataldo · 2016-05-31T00:04:35Z

Seems to me to be more reason to go with the approach @ilyavolodin suggested :)

platinumazure · 2016-05-31T00:14:19Z

Should have mentioned I was able to reproduce the issue with babel-ESLint.
I think astexplorer only shows part of what babel-ESLint does. I only
mentioned it to show my confusion in this area. I'm happy to check for lack
of key and I can amend the PR soon to include that.
On May 30, 2016 7:04 PM, "Kai Cataldo" notifications@github.com wrote:

Seems to me to be more reason to go with the approach @ilyavolodin
https://github.com/ilyavolodin suggested. It might even work for
babel-eslint users as an added bonus.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#6288 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AARWemSdbsPW32THG4TYEIxfeY1QeXpYks5qG3sVgaJpZM4Ip_rq
.

kaicataldo · 2016-05-31T00:36:21Z

Some weirdness with babel-eslint in astexplorer - it actually incorrectly labels RestProperty as SpreadProperty, even though babylon labels it correctly. Might be a bug with babel-eslint.

platinumazure · 2016-05-31T00:38:15Z

I did notice the upper right of astexplorer.net says "acorn-to-esprima
version 1.0.7", maybe that's a clue as to why it's inconsistent on that
site.
On May 30, 2016 7:36 PM, "Kai Cataldo" notifications@github.com wrote:

Some weirdness with babel-eslint in astexplorer - it actually incorrectly
labels RestProperty as SpreadProperty, even though babylon labels it
correctly. Might be a bug with babel-eslint.

[image: screen shot 2016-05-30 at 8 32 22 pm]
https://cloud.githubusercontent.com/assets/7041728/15660147/0f3fcb9a-26a6-11e6-9cfa-aab96ca4605f.png
[image: screen shot 2016-05-30 at 8 32 54 pm]
https://cloud.githubusercontent.com/assets/7041728/15660146/0f3aac32-26a6-11e6-8d18-c953fad7a149.png

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#6288 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AARWenJurlnGtDdXnV6x3xSBqmo6D7ruks5qG4KGgaJpZM4Ip_rq
.

kaicataldo · 2016-05-31T00:39:01Z

Yeah, might be. Regardless, I'm a fan of @ilyavolodin's suggestion!

eslintbot · 2016-05-31T02:09:15Z

LGTM

platinumazure · 2016-05-31T02:10:55Z

@kaicataldo @ilyavolodin Updated the PR per your suggestions. Please let me know if anything else needs fixing.

kaicataldo · 2016-05-31T02:37:06Z

lib/rules/no-useless-rename.js

@@ -87,7 +87,7 @@ module.exports = {
                    return;
                }

-                if (properties[i].computed) {
+                if (properties[i].computed || !properties[i].key) {


Non-blocker: I think a comment here about why this guard is needed could be helpful for future contributors

I'll add that tomorrow. Thanks! Apologies for missing that.

kaicataldo · 2016-05-31T02:38:20Z

One small nit, but otherwise LGTM. Nice work!

ilyavolodin · 2016-05-31T03:21:41Z

LGTM! Thanks.

eslintbot · 2016-05-31T14:11:02Z

LGTM

platinumazure · 2016-05-31T19:47:14Z

The AppVeyor build failure is because AppVeyor still checks multiple commits (see #6292). Please let me know if there are any other issues I'm not aware of.

@kaicataldo Please let me know what you think of the comment I've added.

kaicataldo · 2016-05-31T20:08:44Z

The body of the comment seems fine to me - what does the TODO mean? What's preventing us from checking that now? I ask because I don't think we should push a change if we haven't check everything, and who knows when the next person will actually get to the TODO issue.

platinumazure · 2016-05-31T20:12:24Z

It refers to if babel-eslint ever sends us a SpreadProperty (they do not at the moment) or if the object rest/spread proposal hits Stage 4 and thus espree would now parse that as SpreadProperty (the proposal is in Stage 2). There is nothing to do right now.

kaicataldo · 2016-05-31T20:23:11Z

I see - thanks for explaining. My inclination would be to remove the TODO line

platinumazure · 2016-05-31T22:37:58Z

@kaicataldo Can do- remove without replacement?

kaicataldo · 2016-05-31T22:41:13Z

That's my preference - I think it raises more questions than answers

platinumazure · 2016-05-31T22:43:12Z

Sorry - what I really meant is do you have a better way of expressing it?
If not, I can remove it.
On May 31, 2016 5:41 PM, "Kai Cataldo" notifications@github.com wrote:

That's my preference - I think it raises more questions than answers

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#6288 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AARWej4tOiRKjKE5umTWTcEGsvAp-fMZks5qHLkMgaJpZM4Ip_rq
.

kaicataldo · 2016-05-31T22:49:06Z

My vote is to remove it entirely and cross that bridge if and when we get to it. Others might have a different opinion though!

platinumazure · 2016-05-31T23:04:02Z

@ilyavolodin Thoughts on how to handle the last comment issue here?

ilyavolodin · 2016-06-01T00:16:50Z

I'm fine with removing that comment all together. We don't really create any specific code-path in our rules for babel-eslint. If one of our rules conflicts with it, they usually clone the rule into eslint-plugin-babel and fix it.

platinumazure · 2016-06-01T01:51:14Z

@ilyavolodin So you believe no comment is necessary at all, even beyond removing the TODO? (Sorry for being dense, I just really want to make sure I'm understanding correctly.)

It'd be great to get consensus on this...

ilyavolodin · 2016-06-01T02:29:25Z

Sorry, I meant to remove TODO comment. The rest of the comments are fine.

eslintbot · 2016-06-01T02:39:18Z

LGTM

platinumazure · 2016-06-01T02:41:42Z

Okay, I've removed the TODO. Let me know if there's anything else I need to do.

kaicataldo · 2016-06-01T02:42:56Z

LGTM, thanks!

ilyavolodin · 2016-06-01T14:14:32Z

LGTM

Fix: no-useless-rename handles ExperimentalRestProperty (fixes eslint…

80d4c3c

…#6284)

jquerybot added the CLA: Valid label May 30, 2016

kaicataldo reviewed May 30, 2016
View reviewed changes

Adding output to tests and changing approach

05d6145

kaicataldo reviewed May 31, 2016
View reviewed changes

Adding comment explaining the approach

7a458cf

Removing unnecessary TODO.

5dee706

ilyavolodin merged commit bb69380 into eslint:master Jun 1, 2016

platinumazure deleted the no-useless-rename-crash branch March 20, 2017 18:05

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018

Fix: no-useless-rename handles ExperimentalRestProperty (fixes #6284) #6288

Fix: no-useless-rename handles ExperimentalRestProperty (fixes #6284) #6288

Conversation

platinumazure commented May 30, 2016 • edited

eslintbot commented May 30, 2016

mention-bot commented May 30, 2016

kaicataldo May 30, 2016

Choose a reason for hiding this comment

platinumazure May 30, 2016

Choose a reason for hiding this comment

kaicataldo commented May 30, 2016

mysticatea commented May 30, 2016

ilyavolodin commented May 30, 2016

kaicataldo commented May 30, 2016

platinumazure commented May 30, 2016

kaicataldo commented May 31, 2016 • edited

platinumazure commented May 31, 2016

kaicataldo commented May 31, 2016

platinumazure commented May 31, 2016

kaicataldo commented May 31, 2016

eslintbot commented May 31, 2016

platinumazure commented May 31, 2016

kaicataldo May 31, 2016 • edited

Choose a reason for hiding this comment

platinumazure May 31, 2016

Choose a reason for hiding this comment

kaicataldo commented May 31, 2016

ilyavolodin commented May 31, 2016

eslintbot commented May 31, 2016

platinumazure commented May 31, 2016

kaicataldo commented May 31, 2016 • edited

platinumazure commented May 31, 2016

kaicataldo commented May 31, 2016

platinumazure commented May 31, 2016

kaicataldo commented May 31, 2016

platinumazure commented May 31, 2016

kaicataldo commented May 31, 2016 • edited

platinumazure commented May 31, 2016

ilyavolodin commented Jun 1, 2016

platinumazure commented Jun 1, 2016

ilyavolodin commented Jun 1, 2016

eslintbot commented Jun 1, 2016

platinumazure commented Jun 1, 2016

kaicataldo commented Jun 1, 2016

ilyavolodin commented Jun 1, 2016

platinumazure commented May 30, 2016 •

edited

kaicataldo commented May 31, 2016 •

edited

kaicataldo May 31, 2016 •

edited

kaicataldo commented May 31, 2016 •

edited

kaicataldo commented May 31, 2016 •

edited