-
Notifications
You must be signed in to change notification settings - Fork 2k
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 #2359 fix: avoid "other typed" constructors #2599
Conversation
About the code of the pull-request... The first commit is pretty straightforward, nothing fancy. But the second one is quite ugly IMO. It turned out that a couple of classes on
I didn't change the If someone could give me a hint on how to keep the instantiations as simple |
|
Beautiful work, @epidemian. I agree that the create/wrap/new distinction (I renamed |
Hey @jashkenas, thanks for merging this! 😸 I seems something went wrong in the merge though 😿. Maybe i'm missing something about how grammar changes are handled, but this seems like an oversight. I can fix the grammar.coffee file and push those changes into this branch if you want. Another thing. This change introduces a backward incompatibility. Is it safe to merge it to master as is? Because this could be breaking code that relies on this kind of magic. I think that avoiding the kind of errors reported on #2359 by not doing an implicit |
Thanks for giving it a look. I've just pushed a commit that should fix the bug with the name swaperoo.
Yep, it certainly does.
Not really. We've talked about it a ton -- both here, in Backbone, and elsewhere. And in pretty much every case where you want an "other-typed constructor", a regular function will do just as well, and is far clearer to read. |
@jashkenas, the code introduced in this PR added some entropy to the code base i feel like (i.e. introducing new ways to create AST nodes for some particular kinds of them... and more regex noise in the Jison grammar DSL). Do you think there's a way cleaning that up a little bit so the creation of AST nodes gets more consistent? And do you think it would make sense to make that change? The fix @renekooi suggested does work for the I was thinking of things like adding a |
As a result, the following is no longer valid: class Point
constructor: (x, y) ->
return new Point x, y unless this instanceof Point
@x = x
@y = y Is the above one of the targets of the restriction, or an unfortunate victim? |
This actually allows you to ask for bound constructord with "pretty please" :p. |
@davidchambers IMO, yes, those "power constructors" (do they have googleable name?) would be a target of the restriction. They go hand in hand with one of JavasScript's most magical and nasty "feature": that you can call constructor functions without That being said, it's just my opinion. I'm interested in hearing arguments in favour of them (or a link to an article that explains their benefits). Ping to @jashkenas: any opinion on this matter?
Sorry, i didn't get the joke, care to explain? 😅 |
I'm currently working on a project which makes great use of CoffeeScript's lightweight syntax, as can be seen below: 'LATIN CAPITAL LETTER R':
paths: [
move(0,0), (r 5)(d 1)(r 1)(d 3)(l 1)(d 1)(r 1)(d 3)(l 2)(u 3)
(l 2)(d 3)(l 2)(u 8)
move(2,1), (r 2)(d 3)(l 2)(u 3)
]
Here are the benefits of optional
I'm not sure where I stand on the issue, as I can see advantages to requiring Having reread my comment, I've realized there's a simple way to get all the benefits of optional class Move
constructor: (@x, @y) ->
move = (x, y) -> new Move x, y Having had this realization, I'm now in favour of requiring |
@epidemian it's been turned down several time, but now there's no way to do it in coffee. |
@Nami-Doc Found the previous issues. Lets continue the discussion about new-less constructors there, shall we? @davidchambers Awesome 😸. I was going to propose exactly the same code for a I'm still interested in proposals for how to clean the code changes introduced in this PR 😅 |
just a quick note: @davidchambers [1] #861 (comment) or even simply edit: it should be noted that using |
@epidemian Despite your hard work on this, I think it might be better for us to back it out before 1.5.1. Would you mind figuring out how to gracefully do the revert? Let me know if that's any bother. |
@jashkenas, OK. No, it shouldn't bother really. It's stupid to feel attached to a change that many of us, at the time, agreed to to. It wasn't just a whim of mine. So, back to square one i guess, right? This pull request consists of two commits: one that removes the return statement for auto-generated constructor (the thing i originally tried to fix) and another one that forbids valued returns in constructors altogether. Reverting only the second one would seem like the reasonable thing if we only want to fix #2728; but unfortunately we'll have to revert both commits to make QuickUI work again, as the desired behaviour in that case is for auto-generated constructors to return what the parent constructor returns. That'll leave #2359 as a wont fix i suppose. |
Here's an attempt to fix #2359.
I opened the original issue quite long ago, but procrastinated ad-infinitum after feeling intimidated with
nodes.coffee
. #2596 encouraged me to give it another shot :)This pull request basically re-introduces the ability to extend native objects without defining a constructor:
And also forbids returning any value from the constructor. This throws a SyntaxError:
Empty returns are allowed though:
This is, however, at the expense of losing the ability to define "other typed" constructors. That is, constructors that return objects other than
this
. So it's a backward-incompatible change (one test had to be removed).