X Tutup
The Wayback Machine - https://web.archive.org/web/20200529215314/https://github.com/airbnb/javascript/pull/1682
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

issue #1673 Method chaining #1682

Open
wants to merge 1 commit into
base: master
from
Open

Conversation

@Fatehsandhu
Copy link

Fatehsandhu commented Dec 26, 2017

@ljharb @erights Can you guys let me know if this correct?
Thanks!

Fixes #1673.

Fixes #1673
@Fatehsandhu Fatehsandhu changed the title issue #1673 issue #1673 Method chaining Dec 26, 2017
Copy link

erights left a comment

I like the code rewrite in the comment. Unlike the naive rewrite, it makes the semantics clear.

Regarding the first change, I'd say "Only use" rather than "Use" but I defer to @ljharb on this.

@Fatehsandhu
Copy link
Author

Fatehsandhu commented Dec 26, 2017

@erights Thanks for the prompt reply. I will wait for @ljharb as well.

@@ -1111,6 +1111,8 @@ Other Style Guides
<a name="constructors--chaining"></a><a name="9.3"></a>
- [9.3](#constructors--chaining) Methods can return `this` to help with method chaining.

> When? Use chaining when functions manipulate the internals of an object instead of just returning data.

This comment has been minimized.

Copy link
@ljharb

ljharb Dec 26, 2017

Collaborator

Definitely not "only use", because (for example) all the non-mutating array methods are perfect chaining candidates but do not manipulate any internals.

In general, this section may need to be reworked to suggest making classes immutable (ie, to always return a new instance, allowing chaining and also avoiding mutation). However that shouldn't go in this PR.

README.md Outdated
@@ -1142,7 +1144,8 @@ Other Style Guides
const luke = new Jedi();
luke.jump()
.setHeight(20);
.setHeight(20); // same as var tmp = luke.jump(); tmp.setHeight(20);
// executes left to right. First jump() and then setHeight()

This comment has been minimized.

Copy link
@ljharb

ljharb Dec 26, 2017

Collaborator

is there any reason to believe this comment isn't self-evident?

This comment has been minimized.

Copy link
@Fatehsandhu

Fatehsandhu Dec 26, 2017

Author

@ljharb I agree with you that line 1148 can be removed. Is that okay?

@erights
Copy link

erights commented Dec 26, 2017

I'm ok with removing the comment. The reason I liked it is as an indirect reminder that it does not mean:

luke.jump();
luke.setHeight(20);

but it's probably better just to be explicit about this.

README.md Outdated
@@ -1142,7 +1144,7 @@ Other Style Guides
const luke = new Jedi();
luke.jump()
.setHeight(20);
.setHeight(20); // same as var tmp = luke.jump(); tmp.setHeight(20);

This comment has been minimized.

Copy link
@gergoerdosi

gergoerdosi Dec 26, 2017

Contributor

If this comment stays then it should use const instead of var.

@Fatehsandhu
Copy link
Author

Fatehsandhu commented Dec 26, 2017

@ljharb @erights Any other changes you guys think are appropriate?

@ljharb ljharb force-pushed the Fatehsandhu:master branch from 62dff7f to b6ba1ca Jan 29, 2018
@ljharb ljharb added the editorial label Jul 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.
X Tutup