-
-
Notifications
You must be signed in to change notification settings - Fork 365
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
Feature/add enum attribute #274
Feature/add enum attribute #274
Conversation
1 similar comment
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.
Sorry in advance for being picky on a few things. This looks really promising and good so far, just want to make sure it's solid before merging. Really excited to get this merged in!
docs/_docs/schema.md
Outdated
@@ -122,6 +126,10 @@ function(model) { | |||
} | |||
``` | |||
|
|||
**Enum: Array of string** |
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 should probably be enum
not Enum
, correct? And probably array of strings.
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.
Fixed this in my commit to this PR
oscar.save(function(err) { | ||
should(err).be.null(); | ||
|
||
Dog.$__.table.delete(function () { |
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.
What's the reason for the table.delete
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.
After looking at this closer this now makes sense.
@@ -427,6 +427,11 @@ Attribute.prototype.toDynamo = function(val, noSet, model) { | |||
} | |||
|
|||
if(type.dynamo === 'S') { | |||
if (this.options.enum) { |
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 looks to be in the toDynamo
function. Do we think it would be beneficial to add it to the fromDynamo
function as well? Not sure if it makes sense, what are your thoughts on this?
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.
@brandongoode can you take a look at this and let me know your thoughts? Do you think we should add this to fromDynamo
as well? Should we throw an error in the fromDynamo
if it's not correct in the enum, or?
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.
LGTM
Add enum feature like mongoose one with tests and documentation.
Don't hesitate to tell me if there is something to change or if something is missing.
Fixes #239