Fix: correct docs for Reflect, Reflect cannot take null or undefined,… #7150

sstern6 · 2016-09-13T23:36:09Z

What is the purpose of this pull request? (put an "X" next to item)

[ x] Documentation update

Please check each item to ensure your pull request is ready:

[ x] I've read the pull request guide
I've included tests for my change
I've updated documentation for my change (if appropriate)

What changes did you make? (Give an overview)
Updated the docs that reference Reflect.apply. Reflect.apply's first argument is a target, which according to the mdn specifications is a function.

This PR fixes #7069

mention-bot · 2016-09-13T23:36:11Z

@sstern6, thanks for your PR! By analyzing the annotation information on this pull request, we identified @keithamus, @aubergine10 and @IanVS to be potential reviewers

eslintbot · 2016-09-13T23:36:11Z

Thanks for the pull request, @sstern6! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary must be 72 characters or shorter. Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

eslintbot · 2016-09-13T23:37:45Z

LGTM

kaicataldo · 2016-09-13T23:48:51Z

docs/rules/prefer-reflect.md

@@ -54,12 +54,12 @@ Examples of **correct** code for this rule when used without exceptions:
 ```js
 /*eslint prefer-reflect: "error"*/

-Reflect.apply(undefined, args);
-Reflect.apply(null, args);
+Reflect.apply(obj.fooBar, args);


This and the following line seem to be examples of the exact same use case. Also, doesn't Reflect.apply take three arguments?

It looks to me like maybe the intended examples here were:

Reflect.apply(obj.foo, undefined, args); Reflect.apply(obj.foo, null, args);

and

Reflect.apply(obj.foo, undefined, [arg]); Reflect.apply(obj.foo, null, [arg]);

Youre right, updating

eslintbot · 2016-09-13T23:54:14Z

LGTM

kaicataldo · 2016-09-14T00:04:25Z

docs/rules/prefer-reflect.md

@@ -54,12 +54,12 @@ Examples of **correct** code for this rule when used without exceptions:
 ```js
 /*eslint prefer-reflect: "error"*/

-Reflect.apply(undefined, args);
-Reflect.apply(null, args);
+Reflect.apply(obj.foo, undefined, args);


Oops, sorry, I think these should all be calling foo instead of obj.foo ( i.e. Reflect.apply(foo, undefined, args);) to match the incorrect examples. Can someone else confirm?

IMHO, seems like mdn references Reflect.apply(Math.floor, undefined, [1.5]) so I personally think obj.foo could be used. But i think confirming would be a good idea.

Thanks

I think the above examples of incorrect patterns are badly written (sorry). Above there is foo (a function var) and obj.foo (a function property on an object). So @kaicataldo is right, this below example should demonstrate both - this should be foo.

It might be worthwhile rewriting the above incorrect patterns to use a mixture of names, rather than foo for everything.

what if we use a more suggestive name, like myFunction, or fn?

I'm happy with that. I think potentially foo and obj.foo need different names, e.g. foo can become myFunction and obj.foo obj.myMethod perhaps?

Thanks @keithamus @vitorbal will update per your recommendations.

nzakas · 2016-09-14T17:56:41Z

Just a heads up, the commit message should begin with "Docs:" rather than "Fix:" because this is a documentation fix.

ljharb · 2016-09-15T05:23:59Z

~~I believe this also fixes 7075?~~

sstern6 · 2016-09-15T06:29:05Z

Great, will have this updated by morning.

eslintbot · 2016-09-15T23:56:15Z

LGTM

kaicataldo

Thanks for updating! Two quick things:

I'm fine with changing foo -> myFunction and foo.bar -> obj.myMethod, but I think we should change all the instances of it for that section then (including the examples for the exceptions configuration option). I think it's confusing to not have the corresponding examples share the same names.
Mind adding (fixes #7069) to the end of the commit message? (i.e. Docs: correction in prefer-reflect docs (fixes #7069)) This helps us out a lot because we can track the issue where the change was discussed and also because GitHub will then automatically close the issue for us.

sstern6 · 2016-09-19T18:43:36Z

@kaicataldo Thanks will update!

sstern6 · 2016-09-22T16:28:39Z

@kaicataldo sorry for the delay, should have this back up this afternoon!

kaicataldo · 2016-09-22T17:28:49Z

No worries - thanks for working on this!

eslintbot · 2016-09-27T05:21:10Z

LGTM

eslintbot · 2016-09-27T05:22:23Z

LGTM

sstern6 · 2016-09-27T05:30:21Z

@kaicataldo back up with what I understood to be the changes you requested. Went through the WHOLE file to make sure everything was as clear as possible regarding myFunction and obj.myMethod. Please let me know if I missed anything or if anything needs to be changed based on a misunderstanding. Updated the commit message as well.

Request: Please carefully review the delete section of the file to make sure I understood what was trying to be communicated and I didn't inappropriately change anything.

Question: With regards to Object.getPrototypeOf({}, 'myProp') per the eslint updated docs. It looks like MDN docs state that `Object.getPrototypeOf' only takes the "object whose prototype is to be returned". I am sure it works both ways but wasn't sure if it should be in there for clarity sake. Wanted to confirm with you before making any decisions/additional changes.

Thanks

keithamus · 2016-09-27T09:07:02Z

docs/rules/prefer-reflect.md

 ```

-### Reflect.setPrototypeOf
+### <Reflect class="setPrototypeOf"></Reflect>


This doesn't look right.

Yeah, this shouldn't have changed

kaicataldo

Thanks for working on this! I actually meant that I think this change should be applied to the whole Reflect.apply section (not to the whole file). The initial PR only made changes to the correct examples code block, and I think we should then make the incorrect and exceptions examples match. Sorry if this wasn't clear - does that make sense?

kaicataldo · 2016-09-27T14:37:41Z

docs/rules/prefer-reflect.md

 ```

-### Reflect.setPrototypeOf
+### <Reflect class="setPrototypeOf"></Reflect>


Yeah, this shouldn't have changed

sstern6 · 2016-09-28T01:01:48Z

Apologize @kaicataldo for the confusion. Only the apply section, including the exceptions will be updated with myFunction and obj.myMethod.

Updated file coming your way. Again apologize for my misunderstanding and the confusion.

eslintbot · 2016-09-28T01:03:36Z

LGTM

kaicataldo · 2016-09-28T03:46:48Z

No need to apologize - thanks again for working on this

kaicataldo · 2016-09-28T03:54:59Z

LGTM. Thanks for contributing to ESLint!

jquerybot added the CLA: Valid label Sep 13, 2016

sstern6 force-pushed the issue7069 branch from a1adba6 to 2c76602 Compare September 13, 2016 23:37

kaicataldo reviewed Sep 13, 2016
View reviewed changes

sstern6 force-pushed the issue7069 branch from 2c76602 to a7d7017 Compare September 13, 2016 23:54

kaicataldo reviewed Sep 14, 2016
View reviewed changes

nzakas added the documentation Relates to ESLint's documentation label Sep 14, 2016

ljharb mentioned this pull request Sep 15, 2016

Docs: Reflect.getOwnPropertyNames does not exist #7075

Closed

sstern6 force-pushed the issue7069 branch from a7d7017 to 933c919 Compare September 15, 2016 23:56

kaicataldo suggested changes Sep 18, 2016

View reviewed changes

sstern6 force-pushed the issue7069 branch from 933c919 to 53b0d42 Compare September 27, 2016 05:21

sstern6 force-pushed the issue7069 branch from 53b0d42 to 36aa32b Compare September 27, 2016 05:22

keithamus suggested changes Sep 27, 2016

View reviewed changes

kaicataldo suggested changes Sep 27, 2016

View reviewed changes

Docs: correction in prefer-reflect docs (fixes eslint#7069).

13e55b6

sstern6 force-pushed the issue7069 branch from 36aa32b to 13e55b6 Compare September 28, 2016 01:03

kaicataldo approved these changes Sep 28, 2016

View reviewed changes

kaicataldo merged commit 66adac1 into eslint:master Sep 28, 2016

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: correct docs for Reflect, Reflect cannot take null or undefined,… #7150

Fix: correct docs for Reflect, Reflect cannot take null or undefined,… #7150

sstern6 commented Sep 13, 2016

mention-bot commented Sep 13, 2016

eslintbot commented Sep 13, 2016

eslintbot commented Sep 13, 2016

kaicataldo Sep 13, 2016

sstern6 Sep 13, 2016

eslintbot commented Sep 13, 2016

kaicataldo Sep 14, 2016

sstern6 Sep 14, 2016

keithamus Sep 14, 2016

keithamus Sep 14, 2016

vitorbal Sep 14, 2016

keithamus Sep 14, 2016

sstern6 Sep 14, 2016

nzakas commented Sep 14, 2016

ljharb commented Sep 15, 2016 •

edited

sstern6 commented Sep 15, 2016

eslintbot commented Sep 15, 2016

kaicataldo left a comment

sstern6 commented Sep 19, 2016

sstern6 commented Sep 22, 2016

kaicataldo commented Sep 22, 2016

eslintbot commented Sep 27, 2016

eslintbot commented Sep 27, 2016

sstern6 commented Sep 27, 2016

keithamus Sep 27, 2016

kaicataldo Sep 27, 2016

kaicataldo left a comment

kaicataldo Sep 27, 2016

sstern6 commented Sep 28, 2016 •

edited

eslintbot commented Sep 28, 2016

kaicataldo commented Sep 28, 2016

kaicataldo commented Sep 28, 2016

Fix: correct docs for Reflect, Reflect cannot take null or undefined,… #7150

Fix: correct docs for Reflect, Reflect cannot take null or undefined,… #7150

Conversation

sstern6 commented Sep 13, 2016

mention-bot commented Sep 13, 2016

eslintbot commented Sep 13, 2016

eslintbot commented Sep 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Sep 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzakas commented Sep 14, 2016

ljharb commented Sep 15, 2016 • edited

sstern6 commented Sep 15, 2016

eslintbot commented Sep 15, 2016

kaicataldo left a comment

Choose a reason for hiding this comment

sstern6 commented Sep 19, 2016

sstern6 commented Sep 22, 2016

kaicataldo commented Sep 22, 2016

eslintbot commented Sep 27, 2016

eslintbot commented Sep 27, 2016

sstern6 commented Sep 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaicataldo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstern6 commented Sep 28, 2016 • edited

eslintbot commented Sep 28, 2016

kaicataldo commented Sep 28, 2016

kaicataldo commented Sep 28, 2016

ljharb commented Sep 15, 2016 •

edited

sstern6 commented Sep 28, 2016 •

edited