forked from dbsrgits/sql-translator
-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
Andrew Beverley
committed
Aug 11, 2015
1 parent
d5103d9
commit e31c185
Showing
4 changed files
with
26 additions
and
9 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
e31c185
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.
It's hard to judge the implementation from a cursory glance without a couple of tests showing "input => output".
Also the uniq_keys() function seems to be misplaced - are you sure a map {} .. won't suffice?
e31c185
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.
e31c185
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.
e31c185
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.
e31c185
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've added another couple of commits, primarily to add tests (I found one bug as a result). The overall diff can be seen here: dbsrgits/sql-translator@master...abeverley:master
I can't see any way of removing the uniq_keys() function, without breaking the compatibility of the uniq() function, unless you can?
Is there enough there to comment on? Docs are still missing, as is support for other databases.
Thanks,
Andy
e31c185
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.
@abeverley This looks like the right direction. I am not sure about the actual name of the key
size
, I need to sleep on it for couple days, see if I can figure out why I dislike it.But all in all this looks like the right direction. I will look more closely at the constraint thing when I am about to merge this, which should happen in about a week or so.
Thank you!
e31c185
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.
Thanks for the feedback. I'll aim to tidy it up and add in some of the missing stuff over the next couple of days.
e31c185
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.
Okay, I've tidied it all up, finished it off, and created a PR: dbsrgits#68
If you'd rather the
size
parameter changed to something else then just let me know.Thanks, Andy