www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Can std.conv.toImpl please be deprecated

reply Jack Stouffer <jack jackstouffer.com> writes:
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?
Apr 15 2016
next sibling parent reply "H. S. Teoh via Digitalmars-d" <digitalmars-d puremagic.com> writes:
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.
Apr 16 2016
next sibling parent Seb <seb wilzba.ch> writes:
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.
Apr 17 2016
prev sibling parent Jack Stouffer <jack jackstouffer.com> writes:
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
Apr 17 2016
prev sibling next sibling parent reply WebFreak001 <janju007 web.de> writes:
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
Apr 17 2016
parent Marc =?UTF-8?B?U2Now7x0eg==?= <schuetzm gmx.net> writes:
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));
     }
Apr 17 2016
prev sibling next sibling parent Jonathan M Davis via Digitalmars-d <digitalmars-d puremagic.com> writes:
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
Apr 17 2016
prev sibling next sibling parent reply Jonathan M Davis via Digitalmars-d <digitalmars-d puremagic.com> writes:
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
Apr 17 2016
parent Jack Stouffer <jack jackstouffer.com> writes:
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.
Apr 17 2016
prev sibling parent "H. S. Teoh via Digitalmars-d" <digitalmars-d puremagic.com> writes:
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...
Apr 17 2016