-
Notifications
You must be signed in to change notification settings - Fork 2k
Fix 5085 #5145
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
Fix 5085 #5145
Conversation
Force-pushed small change to the wording of the commit: "Assuming that a bound Code block..." |
$ cake test failed 1 and passed 1274 tests in 4.81 seconds jashkenas#5085: Bug: @ reference to class not maintained in do block AssertionError [ERR_ASSERTION]: Expected undefined to equal 2
Assuming that a bound Code node inside an ExecutableClassBody without a name must be a "do" block.
....and one more force-push to remove an unnecessary step from the test. |
Thanks for this! @connec do you care to take a look? |
@connec or @zdenko or @helixbass, anyone familiar with the |
Can you please add a test for |
@vendethiel Sorry, I don't follow: what do you want the test to do? |
Make sure the function inside the call inside an object is bound to the class (we shouldn’t attribute a name outside of class properties IIRC so the test should already work). |
jashkenas#5145 (comment) Also switched to concat'g strings for test values, as opposed to incrementing numbers: makes the tests a bit easier to read.
I'm not totally sure if this is what you meant...? If it's not, could you just write the exact code you mean? Sorry that I'm just not getting it. |
|
jashkenas#5145 (comment) Also switched to concat'g strings for test values, as opposed to incrementing numbers: makes the tests a bit easier to read.
222067d
to
dfbac63
Compare
I don't understand what this is doing at all, or what the intent of writing it would be, but OK, test added. :-) Is |
There was an old bug where we considered object properties outside of the class top-level and gave them names, just adding a test. |
So @vendethiel is this okay to merge? |
LGTM |
Thanks @brewingcode! |
You're welcome! |
Thanks for all the great work with CoffeeScript, I was just trolling through the issues and this looked like a simple fix. The fix is based on an assumption that I could be completely wrong about, but according to the tests (including the new test for the bug itself), the assumption is correct.