leerho commented on code in PR #59: URL: https://github.com/apache/datasketches-rust/pull/59#discussion_r2662710652
########## datasketches/src/lib.rs: ########## @@ -38,8 +38,11 @@ pub mod hll; pub mod tdigest; pub mod theta; +mod binomial_bounds; Review Comment: When we started out with only one or two sketches we had all these shared constants and methods at the root and that became a real mess. So some years ago we moved it all to /common/. (As it turns out, that name is used by multiple libraries.) Later, in Java, I split it into 3 groups: /common/, /thetacommon/ and /quantilescommon/. The theta, tuple and quantiles sketches are the most complex and because Java generics perform so poorly, I created dedicated sketch implementations for _longs_, _floats_, and _doubles_ and then an _items_ generic class for everything else. Of course C++ doesn't have this problem and I presume rust doesn't either. Generally, whatever naming you decide is fine with me. You will want to follow standard practice for the Rust language. However, keep in mind that making arbitrary changes in naming will make it harder for folks to examine how something is implemented across languages, which will make any troubleshooting more difficult as well. Perhaps the names we chose were not the best choices, but they are what they are. I would strongly discourage leveraging a constant defined in one sketch in a different sketch. This is what the /common/ namespaces are for. If you discover that we have done that in our current languages, please let us know, it is a bug. At some point in the future, we may want to be able to allow the user to just load the sketch(s) that they need and not have to load the entire library. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
