Implement #1495, Method call chaining #3263

Merged
merged 2 commits into from Nov 28, 2013

Conversation

Projects
None yet
10 participants
@xixixao
Contributor

xixixao commented Nov 27, 2013

Implements #1495 and parts of its sibling issues.

@jashkenas was against this 3 years ago saying:

This has come up before, and I'm still of the opinion that it's an abuse of indentation. Leading dot accessors should attach to the final value of the previous line, and not cause the previous line to be implicitly wrapped in parentheses.

This sounds reasonable, but in practice I was unable to find any occurrences of the pattern

a b
.c() #a(b.c())

vs hundreds of cases of

a.b(c)
 .d(e)

This has also been highly requested and discussed syntax. There have been many proposals, a lot of them dealing with indentation. I think that we can agree that the change should be as simple and useful as possible.

There are several examples already where our code looks similar to the proposal:

a b,
  c
.something() # a(b, c).something()

a
  b: c
.something()  # a(b: c).something()

a ->
  b c
.something()  # a(function () {return b(c);}).something()

This PR brings normal argument list on par with these, and also solves cases of nested calls and inline functions. This is now valid:

result = range 1, 3
  .concat range 4, 6
  .map (x) -> x * x
  .filter (x) -> x % 2 is 0

console.log result # [4, 16, 36]

Yet the syntax is indentation agnostic. The rule is simply that a dot accessor on a new line closes all implicit calls, or that it "attaches to the outermost preceding value".

oldschool P.S.:

$ 'body'
.click (e) ->
  $ '.box'
  .fadeIn 'fast'
  .addClass '.active'
  .css 'marginRight', '10px'
@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Nov 27, 2013

Collaborator

Wow, I didn't think we'd ever see a PR for this. Nice job, I'll review soon.

Collaborator

michaelficarra commented Nov 27, 2013

Wow, I didn't think we'd ever see a PR for this. Nice job, I'll review soon.

@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Nov 27, 2013

Owner

The rule is simply that a dot accessor on a new line closes all implicit calls

All of em?

What happens in cases like this?

function arg, ->
  jQuery selector
    .fadeIn 'slow'
Owner

jashkenas commented Nov 27, 2013

The rule is simply that a dot accessor on a new line closes all implicit calls

All of em?

What happens in cases like this?

function arg, ->
  jQuery selector
    .fadeIn 'slow'
@vendethiel

This comment has been minimized.

Show comment
Hide comment
@vendethiel

vendethiel Nov 27, 2013

Collaborator

@jashkenas I had the same question, but then I re-read the last example :

$ 'body'
.click (e) ->
$ '.box'
.fadeIn 'fast'
.addClass '.active'
.css 'marginRight', '10px'

Collaborator

vendethiel commented Nov 27, 2013

@jashkenas I had the same question, but then I re-read the last example :

$ 'body'
.click (e) ->
$ '.box'
.fadeIn 'fast'
.addClass '.active'
.css 'marginRight', '10px'

@xixixao

This comment has been minimized.

Show comment
Hide comment
@xixixao

xixixao Nov 27, 2013

Contributor

@jashkenas Only implicit calls, not function bodies or object bodies. So in your case the "open" function body prevents closing of the first function call. I agree explaining it in words is tougher than showing by examples / the algorithm itself, but it should be easy to grasp.

Contributor

xixixao commented Nov 27, 2013

@jashkenas Only implicit calls, not function bodies or object bodies. So in your case the "open" function body prevents closing of the first function call. I agree explaining it in words is tougher than showing by examples / the algorithm itself, but it should be easy to grasp.

@nfour

This comment has been minimized.

Show comment
Hide comment
@nfour

nfour Nov 28, 2013

+1. The current trends with promises makes using coffeescript somewhat pointless when chaining as we need to wrap with parenthesis or assign temp variables.

Chaining like this would be highly pleasing, seems like one of the few remaining JS syntax constructs CS is failing to improve on. I could imagine it would be more fitting for redux though considering backwards compat.

nfour commented Nov 28, 2013

+1. The current trends with promises makes using coffeescript somewhat pointless when chaining as we need to wrap with parenthesis or assign temp variables.

Chaining like this would be highly pleasing, seems like one of the few remaining JS syntax constructs CS is failing to improve on. I could imagine it would be more fitting for redux though considering backwards compat.

@xixixao

This comment has been minimized.

Show comment
Hide comment
@xixixao

xixixao Nov 28, 2013

Contributor

I could imagine it would be more fitting for redux though considering backwards compat.

If we were using semver we would be on version 7.0.1. There are loads of backwards incompatible changes (the changes to string literals I worked on for example) on master. I also hope that #2887 will get merged in before 1.7.0 (more of a big change rather than backwards incompatible). The main point is I couldn't find the code that changes behavior in production, any examples? Redux is not tangible for me right now.

I totally agree about promises.

Contributor

xixixao commented Nov 28, 2013

I could imagine it would be more fitting for redux though considering backwards compat.

If we were using semver we would be on version 7.0.1. There are loads of backwards incompatible changes (the changes to string literals I worked on for example) on master. I also hope that #2887 will get merged in before 1.7.0 (more of a big change rather than backwards incompatible). The main point is I couldn't find the code that changes behavior in production, any examples? Redux is not tangible for me right now.

I totally agree about promises.

@nfour

This comment has been minimized.

Show comment
Hide comment
@nfour

nfour Nov 28, 2013

@xixixao I simply imagine backwards compatibility would be the only practical obstacle for this feature. I've followed this chaining syntax debate for a year or so and the only other reason that comes to mind is that the big CS contributors just don't like it, or care. I'd imagine their opinions may have changed, though. Heres to hoping :D

nfour commented Nov 28, 2013

@xixixao I simply imagine backwards compatibility would be the only practical obstacle for this feature. I've followed this chaining syntax debate for a year or so and the only other reason that comes to mind is that the big CS contributors just don't like it, or care. I'd imagine their opinions may have changed, though. Heres to hoping :D

@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Nov 28, 2013

Owner

Here we go...

Owner

jashkenas commented Nov 28, 2013

Here we go...

jashkenas added a commit that referenced this pull request Nov 28, 2013

Merge pull request #3263 from xixixao/issue1495
Implement #1495,  Method call chaining

@jashkenas jashkenas merged commit 563f14b into jashkenas:master Nov 28, 2013

@raganwald

This comment has been minimized.

Show comment
Hide comment

👍

@charltoons

This comment has been minimized.

Show comment
Hide comment

👍 👍

+ endAllImplicitCalls = ->
+ while inImplicitCall()
+ endImplicitCall()
+

This comment has been minimized.

@shesek

shesek Dec 5, 2013

There should probably be a return here, to avoid storing/returning the value of the comprehension.

@shesek

shesek Dec 5, 2013

There should probably be a return here, to avoid storing/returning the value of the comprehension.

vendethiel added a commit that referenced this pull request Dec 5, 2013

Merge pull request #3276 from xixixao/issue1495fixup
Fixup #3263: Prevent loop collection in endAllImplicitCalls
@00dani

This comment has been minimized.

Show comment
Hide comment
@00dani

00dani Dec 6, 2013

This looks great! When can we expect to see a release containing this change? It'll definitely work wonderfully for promises, as well as a bunch of other chainy interfaces (jQuery's for example).

00dani commented Dec 6, 2013

This looks great! When can we expect to see a release containing this change? It'll definitely work wonderfully for promises, as well as a bunch of other chainy interfaces (jQuery's for example).

@shesek

This comment has been minimized.

Show comment
Hide comment
@shesek

shesek Dec 6, 2013

I think I have some cases in my code-base that relied on the old behavior. Can someone perhaps make some tool to find those cases, or possibly even issue a warning with the file/line and a notice about what changed when CoffeeScript encounter those cases in the next 2-3 releases, until everyone are aware of it?

I can see this causing some havoc... we at least need an easy way to let people find those cases before they upgrade.

shesek commented Dec 6, 2013

I think I have some cases in my code-base that relied on the old behavior. Can someone perhaps make some tool to find those cases, or possibly even issue a warning with the file/line and a notice about what changed when CoffeeScript encounter those cases in the next 2-3 releases, until everyone are aware of it?

I can see this causing some havoc... we at least need an easy way to let people find those cases before they upgrade.

@xixixao

This comment has been minimized.

Show comment
Hide comment
@xixixao

xixixao Dec 6, 2013

Contributor

@shesek I'd simply compile your codebase with both versions (you can do this now with last release and master) and diff the outputted JS. I'd be interested to see the use cases.

Contributor

xixixao commented Dec 6, 2013

@shesek I'd simply compile your codebase with both versions (you can do this now with last release and master) and diff the outputted JS. I'd be interested to see the use cases.

@simoami

This comment has been minimized.

Show comment
Hide comment
@simoami

simoami Jan 8, 2014

glad to see method chaining finally happening. Thanks!

simoami commented Jan 8, 2014

glad to see method chaining finally happening. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment