-
Notifications
You must be signed in to change notification settings - Fork 288
Conversation
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.
Nice! This is a really elegant implementation. The direction definitely looks good to me
Would love to see those type tests you mentioned in the opening description to make sure it behaves. Probably we can have a UUIDFaker
or something. We might need both a "string" and "uuid" type category to test this there?
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.
Based on the replies to comments, whenever you've made the changes you find appropriate to address them, go for it 👍🏻
@sirupsen This PR is ready. I added a few more changes, but nothing too surprising. Still, thought I'd give you a chance to review it. |
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.
Looking good Erez!! Let's get them to check this out in the issue!
Only some small comments that I'll leave at your discretion, won't need to look again
Currently only working for MySQL, but I'm posting it for comments about my approach and choices.
Next steps would be to -
Add more tests. Maybe into test_database_types
Support more databases and column types.