-
Notifications
You must be signed in to change notification settings - Fork 60
Support UUID type in msgpack #104
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
Conversation
ed87373
to
67374a5
Compare
Could you place it into subfolder (subpackage)? I don't like it in main package. |
What's the motivation? UUID is built-in in Tarantool. |
Yep. But it doesn't built-in in Golang. Why github.com/google/uuid is better than any other?
Looks like satori's one is more popular than google's one. |
Well, I don't think it is better than any other package, but AFAIK it doesn't worse than any of them since they all implement similar basic functionality. Regarding popularity, satori/go.uuid is a bit more popular than google/uuid in terms of GitHub stars (4.4k vs 3.2k), but google/uuid leaves behind satori/go.uuid in case of Yeah, if someone is willing to use a Tarantool connector with application that already uses another uuid package, it can introduce a conflict of namespaces. I'll try to rework code to avoid it. |
Yep, I'm not against google/uuid. And I'm not promoting any particular uuid package. I'm against including any particular uuid in main package-folder. Golang provocates using subpackages (subfolders) for anything optional. And I think it is right way. And dependency on particular uuid package is optional. |
Moved to submodule in new commit |
I've noticed that without importing |
Hi! Are you suggesting moving the uuid into a subpackage just to decorate the project structure or something else? I don't mind, just interesting. |
Hi! Thank you for the patch. Generally ok.
|
Well, since uuid was extracted to submodule after review, you can say that it is intentional, but discussable |
30e71dd
to
c1960a0
Compare
|
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, but I am not very familiar with the project and the rules for formatting the code and commits in it, so I could not take into account some points
536e750
to
68a6b1c
Compare
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.
Do you plan to squash commits (it seems worthful)? Shouldn't we drop github.com/google/uuid from go.mod in the second commit (and move it to go.mod inside uuid/
)? Or it does not work this way?
I have a couple of questions, but generally can't add a value here. LGTM, but my review is unsufficient.
68a6b1c
to
6551d76
Compare
Yes, I changed it in a last force-push.
Thank you, it definitely should be like this. Fixed. |
This patch provides UUID support for all space operations and as function return result. UUID type was introduced in Tarantool 2.4.1. See more in commit messages [1]. To use UUID with google/uuid in msgpack, import tarantool/go-tarantool/uuid submodule. 1. tarantool/tarantool@d68fc29 Closes #90
6551d76
to
fd68e12
Compare
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 go.mod changes a necessary. But nonetheless, approve.
This patch provides UUID support for all space operations and as function return result. UUID type was introduced in Tarantool 2.4.1. See more in commit messages.
To use UUID with google/uuid in msgpack, import tarantool/go-tarantool/uuid submodule.
Closes #90