Skip to content

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

Merged
merged 3 commits into from
Feb 11, 2019
Merged

Fix 5085 #5145

merged 3 commits into from
Feb 11, 2019

Conversation

brewingcode
Copy link
Contributor

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.

@brewingcode
Copy link
Contributor Author

brewingcode commented Jan 7, 2019

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.
@brewingcode
Copy link
Contributor Author

....and one more force-push to remove an unnecessary step from the test.

@GeoffreyBooth
Copy link
Collaborator

Thanks for this! @connec do you care to take a look?

@GeoffreyBooth
Copy link
Collaborator

@connec or @zdenko or @helixbass, anyone familiar with the Class node have a minute to review a one-line fix?

@vendethiel
Copy link
Collaborator

Can you please add a test for class A then fn(b: => ...)?

@brewingcode
Copy link
Contributor Author

@vendethiel Sorry, I don't follow: what do you want the test to do?

@vendethiel
Copy link
Collaborator

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).

brewingcode added a commit to brewingcode/coffeescript that referenced this pull request Feb 10, 2019
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.
@brewingcode
Copy link
Contributor Author

I'm not totally sure if this is what you meant...?

222067d

If it's not, could you just write the exact code you mean? Sorry that I'm just not getting it.

@vendethiel
Copy link
Collaborator

myvar = 0
fn = (o) -> o.prop()
class Klass
  @foo = 1
  fn prop: => myvar = @foo

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.
@brewingcode
Copy link
Contributor Author

class Klass
  fn prop: => myvar = @foo

I don't understand what this is doing at all, or what the intent of writing it would be, but OK, test added. :-)

Is myvar supposed to be 1 after this...?

@vendethiel
Copy link
Collaborator

There was an old bug where we considered object properties outside of the class top-level and gave them names, just adding a test.

@GeoffreyBooth
Copy link
Collaborator

So @vendethiel is this okay to merge?

@vendethiel
Copy link
Collaborator

LGTM

@GeoffreyBooth GeoffreyBooth merged commit 63ffe0a into jashkenas:master Feb 11, 2019
@GeoffreyBooth
Copy link
Collaborator

Thanks @brewingcode!

@brewingcode brewingcode deleted the fix-5085 branch February 11, 2019 17:30
@brewingcode
Copy link
Contributor Author

You're welcome!

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.

3 participants