On Friday, April 15, 2016 17:23:26 Jack Stouffer via Digitalmars-d wrote: > Before I opened a PR, I wanted to get some second opinions. > > There is no reason IMO that the various overloads of toImpl > should be public. Having the internal functionality of a parent > function, in this case to, be exposed like this causes: > > 1. The docs to be cluttered with useless info. Anything pertinent > can be moved to the to docs. > 2. The function cannot be refactored because its guts are shown > to the world > > Also, there is no reason that anyone should use toImpl over to. > So can I please mark toImpl as deprecated in order to clean up > std.conv?
What do you propose that we do instead? We need all of the separate toImpl functions regardless of the name. Do you want to try and embed them all inside of std.conv.to with the name to? I expect that it's feasible, but it would royally suck from the standpoint of unit tests, since then either 1. you end up with all of them inside of the template to, which is very bad - it would result in a ton of unit test duplication (including duplicating those tests in the code of any program which instantiated to), and you'd have to make sure to have a unit test outside of std.conv.to which instantiated it so that the tests actually got run. 2. Or you're forced to put all of the unit tests outside of std.conv.to so that they're not next to the functions being tested, which is maintenance nightmare. So, unless some form of DIP 82 (http://wiki.dlang.org/DIP82) is implmemented so that we can sanely put unit test blocks inside of a template, I think that it's a huge mistake to try and embed each of the toImpl overloads inside of std.conv.to. Sure, toImpl is a bit ugly, but it really doesn't cost us much from what I can tell. And unless you can come up with a way that allows us to have all of those overloads named to while retaining the unit test blocks after each function that they go with and _not_ putting any of them inside of a template, I don't think that it's even vaguely worth fixing. If/When DIP 82 is implemented, then sure, put them all inside of the main std.conv.to template if you can, but for now, please don't. - Jonathan M Davis