-
Notifications
You must be signed in to change notification settings - Fork 14
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
Support for Enum Types #18
base: master
Are you sure you want to change the base?
Conversation
@@ -1,7 +1,7 @@ | |||
import test from "./test" | |||
import * as Immutable from "immutable" | |||
import {Record} from "../record" | |||
import {Typed, typeOf, Union, Range, Maybe} from "../typed" |
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.
this was not used in the tests
Unfortunately I am no longer able to maintain this project, I apologize. If someone wishes to step up and take leadership of this project please respond to #21 |
|
||
|
||
return Typed(`Enum(${validValues})`, value => { | ||
if (!map[value] && value !== undefined){ |
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.
I don't think the value !== undefined
check should be there - if user want's an optional value shouldn't they choose to use Maybe
?
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 call. will change that.
Do we want this in the main library? It's trivial to just make a That said, probably this is a pretty common use case, and deserves inclusion. |
Seems common enough that it's probably worth including - I've added something equivalent on every project I've used typed-immutable on. |
I like this too, seems like a consensus. @udnisap can you change the |
@udnisap Can you merge in |
Sorry, we need docs for this as well |
@stutrek had to refactor the tests and also to add the first value as the default value. |
Great @udnisap! We will need to add this to the readme. Let me know if you need any help with that. |
@udnisap I am ready to cut a release as soon as this is readme-d and ready. |
No description provided.