Skip to content
This repository was archived by the owner on May 17, 2024. It is now read-only.

Support for UUID key column #119

Merged
merged 10 commits into from
Jun 29, 2022
Merged

Support for UUID key column #119

merged 10 commits into from
Jun 29, 2022

Conversation

erezsh
Copy link
Contributor

@erezsh erezsh commented Jun 28, 2022

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.

@erezsh erezsh requested a review from sirupsen June 28, 2022 12:00
Copy link
Contributor

@sirupsen sirupsen left a 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?

Copy link
Contributor

@sirupsen sirupsen left a 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 👍🏻

@erezsh erezsh marked this pull request as ready for review June 29, 2022 12:16
@erezsh
Copy link
Contributor Author

erezsh commented Jun 29, 2022

@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.

Copy link
Contributor

@sirupsen sirupsen left a 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

@erezsh erezsh merged commit d7ae027 into master Jun 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants