-
-
Notifications
You must be signed in to change notification settings - Fork 95
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
add S.create for creating Sanctuary modules with custom environments #206
Conversation
Played around with this for a bit. Because of sanctuary-js/sanctuary-def#52, sanctuary already tolerates many things not in its environment, right? Some notes while playing around:
var Car = function(make, model) {...}
Car.prototype = {...} can I do
const Point = $.RecordType({x: Number, y: Number});
S = S.create(true, S.env.concat([Point]));
let p = Point(10, 20); I think this would also help sanctuary-def know that it actually received a type rather than just some StrMap if the keys are homogenous? |
Another thought: a section about using sanctuary and sanctuary-def with and without eachother (of course sanctuary itself depends on sanctuary-def, but from a users perspective). Can I use them alone? What would that look like? Why might I? Do they play well together? What does
mean? etc.. |
Thank you for the thoughtful feedback, Kevin. The reason I haven't yet responded is that I haven't yet had time to write an equally thoughtful response. |
d7ce861
to
9439211
Compare
⚡ |
52918bd
to
fb4fee7
Compare
Understanding The position of the "Type Checking" section is appropriate for what I consider to be advanced material.
I love this suggestion. I've added such an example to the
Perhaps it's better to cover this in the sanctuary-def documentation. What do you think?
In most cases custom types needn't be included in the environment, as the environment is only necessary for resolving type variables. I'd like users to assume that the default environment is fine, and only provide a custom environment if the need arises. This pull request is ready for final review. We're close to a v0.11.0 release. :) |
Good old identity :) It's a bit hard for me to imagine how it will all look without seeing it on the site, but I like it.
I think you are probably right on both counts. I just kind of get the feeling I want to really understand sanctuary-def to better use sanctuary. Something like the sanctuary-def docs would be pretty dry, API-like, while sanctuary.org really explains everything, including sanctuary-def. It is difficult though to balance getting started vs kitchen sink. In any event, I'm not trying to delay 0.11, I'm sure we don't need to really nail down where all this stuff should go to get it out the door thumbs up! |
|
||
it('returns a Sanctuary module', function() { | ||
var expected = R.keys(S).sort(); | ||
eq(R.keys(checkedDefaultEnv).sort(), expected); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should S.keys here. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't actually use S.keys
here, since S
is not a member of StrMap a
. I've replaced R.keys
with Object.keys
(which reads nicely for getting the names of a record's fields).
I've looked over the code of this PR many times (every time I get a notification) and started writing this comment almost as often. Every time I try I end up writing massive comments that when proof reading seem to always diverge too much from the subject matter and have no clear point. I keep deciding in the end not to post. I should though, because @davidchambers is waiting for a second reviewer. I'll try to summarize:
|
cf95fec
to
ef24b9d
Compare
Thanks for your comment, @Avaq.
Sounds great! The thoughts you've shared regarding extensible environments are intriguing. |
I took the time to write down those ideas I mentioned: sanctuary-js/sanctuary-def#74 |
Closes #188
I'd like to receive feedback from at least two people before merging this pull request, as this is a significant API change.