-
Notifications
You must be signed in to change notification settings - Fork 11
feat: add union operation of theta sketches #64
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
| ) | ||
|
|
||
| // Union computes the union of Theta sketches. | ||
| type Union struct { |
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 mind naming this ThetaUnion (Union is too generic and this get confusing on the call site when mixing different sketch type)
(I probably missed other case like that, I ll do a pass to qualify those exported types)
Thanks
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.
Some context here, the name colision thing is a larger problem - for the Go repo it's going to be easier to address now than later
https://lists.apache.org/thread/4c78gsls7xjrn0b4cosvppywh2clwz6x
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.
Some context here, the name colision thing is a larger problem - for the Go repo it's going to be easier to address now than later
Yes, I agree that. But In go, always use package as accessor. And even if packages are conflicted, user can change package name using alias when import.
refs:
https://go.dev/wiki/CodeReviewComments#package-names
https://go.dev/blog/package-names#naming-package-contents
https://go.dev/doc/effective_go#package-names
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.
But adding prefix is project or community rule, I will follow it. Do you want still change 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.
Lets go with Go convention here - thanks
Adding Union operation of theta sketches.