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?

On Fri, Apr 15, 2016 at 05:23:26PM +0000, 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?

I'm pretty sure that toImpl being public is an oversight. The name itself implies that it should be private.  I seriously doubt any user code actually calls toImpl directly... shouldn't it be just a matter of marking it private instead?  Do we really need to go through a deprecation cycle for this?


T

-- 
Tech-savvy: euphemism for nerdy.

On Sunday, 17 April 2016 at 05:04:46 UTC, H. S. Teoh wrote:
> On Fri, Apr 15, 2016 at 05:23:26PM +0000, Jack Stouffer via Digitalmars-d wrote:
>> [...]
>
> I'm pretty sure that toImpl being public is an oversight. The name itself implies that it should be private.  I seriously doubt any user code actually calls toImpl directly... shouldn't it be just a matter of marking it private instead?  Do we really need to go through a deprecation cycle for this?
>
>
> T

+1 for avoiding the depreciation cycle and directly setting it to private.

On Friday, 15 April 2016 at 17:23:26 UTC, Jack Stouffer 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?

you need to use toImpl if you want to convert a number to/from a specific base! I use that a lot when converting hexadecimal values

On Sunday, 17 April 2016 at 09:22:21 UTC, WebFreak001 wrote:
> you need to use toImpl if you want to convert a number to/from a specific base! I use that a lot when converting hexadecimal values

No, the documentation just gives that impression. This works:

    void main(string[] args) {
        import std.conv;
        import std.stdio;
        writeln(args[1].to!int(10).to!string(16));
    }

On Saturday, April 16, 2016 22:04:46 H. S. Teoh via Digitalmars-d wrote:
> On Fri, Apr 15, 2016 at 05:23:26PM +0000, 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?
>
> I'm pretty sure that toImpl being public is an oversight. The name itself implies that it should be private.  I seriously doubt any user code actually calls toImpl directly... shouldn't it be just a matter of marking it private instead?  Do we really need to go through a deprecation cycle for this?

toImpl has the documentation for all of the specific conversions. So, you can't make it private or that documentation would go away.

- Jonathan M Davis

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

On Sun, Apr 17, 2016 at 03:32:16AM -0700, Jonathan M Davis via Digitalmars-d wrote:
> On Saturday, April 16, 2016 22:04:46 H. S. Teoh via Digitalmars-d wrote:
> > On Fri, Apr 15, 2016 at 05:23:26PM +0000, 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?
> >
> > I'm pretty sure that toImpl being public is an oversight. The name itself implies that it should be private.  I seriously doubt any user code actually calls toImpl directly... shouldn't it be just a matter of marking it private instead?  Do we really need to go through a deprecation cycle for this?
> 
> toImpl has the documentation for all of the specific conversions. So, you can't make it private or that documentation would go away.
[...]

Easy, just move that documentation to the docs for `to`.

It's probably better that way anyway, as exemplified by this very thread where somebody already expressed confusion over using toImpl with a conversion base vs. using to with a conversion base, where the latter is probably the original intended usage.


T

-- 
I think the conspiracy theorists are out to get us...

On Sunday, 17 April 2016 at 11:00:33 UTC, Jonathan M Davis wrote:
> 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

If we do as H. S. Teoh is suggesting and go straight to marking as private, then there is no reason to.

On Sunday, 17 April 2016 at 05:04:46 UTC, H. S. Teoh wrote:
> I'm pretty sure that toImpl being public is an oversight. The name itself implies that it should be private.  I seriously doubt any user code actually calls toImpl directly... shouldn't it be just a matter of marking it private instead?  Do we really need to go through a deprecation cycle for this?

https://github.com/dlang/phobos/pull/4208