KEMBAR78
#1234 Protoype Access in :: Operator by geraldalewis · Pull Request #1590 · jashkenas/coffeescript · GitHub
Skip to content

Conversation

geraldalewis
Copy link
Contributor

Issue #1234: Applying a splat to a prototype using :: applies the splat to the wrong object
Opened by: @Dykam
Date opened: March 25, 2011
Milestone: 1.2

Thanks to @michaelficarra for pointing to a solution and review!

Dykam's illustrative code:

A.prototype.method array... # Compiles correctly
A::method array... # Compiles incorrectly

var _ref;
(_ref = A.prototype).method.apply(_ref, array); // Correct
A.prototype.method.apply(A, array); // Incorrect

Solution: Adding an intermediary prototype Access node between the class and the method within the grammar.

I have some questions regarding some possible refinements posted in the src/ commit.

This issue ties into a larger issue on how :: is handled ( with respect to @ ) -- I will open a new issue that addresses the super-issue within two days.

@geraldalewis
Copy link
Contributor Author

Here's a full diff of the entire patch (since the diff in Commits <> only shows the changes between the last two commits).

@Dykam
Copy link

Dykam commented Aug 12, 2011

Nice to see progress :] I wonder, why do you gist a diff and not commit to a fork?

@geraldalewis
Copy link
Contributor Author

@Dykam I will if that makes life easier for the collabs or owner; just didn't seem like a big enough change to warrant another branch?

@michaelficarra
Copy link
Collaborator

I see all 3 commits and a combined diff. No idea what you guys are talking about.

@Dykam
Copy link

Dykam commented Aug 12, 2011

The diff didn't get on the mail, so I looked over it, never mind.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be a little less confusing:

class C
  method: -> @nonce
  nonce: nonce

arr = []
eq nonce, C::method arr... # should be applied to `C::`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes -- that looks better. I'll update in a moment :)

@michaelficarra
Copy link
Collaborator

Awesome patch, @geraldalewis. LGTM.

@geraldalewis
Copy link
Contributor Author

Thanks to you @michaelficarra :)

@jashkenas
Copy link
Owner

Gorgeous patch.

jashkenas added a commit that referenced this pull request Aug 12, 2011
@jashkenas jashkenas merged commit 42f2bd9 into jashkenas:master Aug 12, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants