Closed safareli closed 7 years ago
I'd like to see List.Nil
rather than List.Nil()
. I made this change for union-type in paldepind/union-type#55.
@davidchambers I had the ()
in toString
only, in code u do like this List.Cons('1', List.Nil)
.
anyways using ()
in toString is not "correct" , will fix that shortly
I'm personally happy with the performance of it currently, so don't go mental going optimisation mad imo. Also we should be careful what is an optimisation now, might not be one in the near future and so push back on micro-optimisations where it harms readability (note: i'm not saying any suggests fall into this category, just we have to be aware).
Current state looks like this:
@Avaq thanks for review.
+=
and it has no significant improvements using the benchmark we have in this pr. (I'm using node 7)makeValue
should take array as it stores it in @@values
so we can't do anything unless we can extend Function
. also we need to define some props on the function, but current improvement from old version is already pretty good.It would be nice if you folks, play around with this version.
btw is the is
name ok?
I think is
fits perfectly!
I have updated readme but i'm not sure if textual description of thouse methods are descriptive/correct enough. would <3 suggestions there. here is rendered version of readme: https://github.com/safareli/daggy/tree/is-new
Love it, I think it's good to go, we can always update it if someone spots anything. Let me know if you're done and I'll merge (tomorrow and move to FL).
On Mon, 30 Jan 2017, 19:39 Irakli Safareli, notifications@github.com wrote:
I have updated readme but i'm not sure if textual description of thouse methods are descriptive/correct enough. would <3 suggestions there. here is rendered version of readme: https://github.com/safareli/daggy/tree/is-new
— You are receiving this because you commented.
Reply to this email directly, view it on GitHub https://github.com/puffnfresh/daggy/pull/9#issuecomment-276167363, or mute the thread https://github.com/notifications/unsubscribe-auth/ACcaGByhSsxpB5FVqt5dADfprLukwmBcks5rXjyGgaJpZM4Lw67U .
before
toString
was too fast in old version of daggy as it didn't defined one, now new version is pretty close.
@SimonRichardson ready for merge 🌮
@puffnfresh can you move this repo over to FL for me, I don't have access to the setting page :pray:
@puffnfresh ping
fix #7
This implementation has some breaking changes:
Also you can't call
List
(which before throwed) as it's not just an object representing the typeList
.We also have some performance improvements, main one is that cata is about two times faster:
New implementation is slower in some cases:
constructor
is created as old version was delegating all work to the created constructor function, when new implementation isn't. but in combined cases when type is constructed and then value is created new version is fasterAlso we can make construction of objects much faster by not using Object.defineProperty but then
@@tag
and@@values
will be visible in node REPL, and I think infor in
as well.Would love your comments/suggestions.
TODO:
is
:hasInstance
,isVariant
)Benchmarks are run on
2.5 GHz Intel Core i7
mac usingnode v7.0.0
README is updated by hand as
emu
is failing onnode 7
maybe because ofes6
, but i just removed it: