[docs] Improve docs for std.conv.to #4900

ntrel · 2016-11-14T13:54:59Z

Document overloads of to which take a radix - see last commit.

Note: This continues the work done in #4208.

JackStouffer · 2016-12-06T15:03:05Z

Have to say that I'm against these changes.

Before I changed the to docs to what they are now, it was a jumbled mess of all of the different overloads of toImpl for every type. The idea with how it's arranged now is that the particulars of each type to type conversion would be under one place. The fact that the radix overloads exist is already listed under the integer conversion examples.

ntrel · 2016-12-15T12:07:54Z

The radix overload docs are currently buried in the middle of all the other to docs. It seems to me that they are significantly different because they take an extra argument, and should be documented after the usual to overloads. It is also non-standard to document the radix overloads without showing their signature, only their usage.

wilzbach · 2017-02-28T08:06:11Z

Have to say that I'm against these changes.
Before I changed the to docs to what they are now, it was a jumbled mess of all of the different overloads of toImpl for every type. The idea with how it's arranged now is that the particulars of each type to type conversion would be under one place. The fact that the radix overloads exist is already listed under the integer conversion examples.

We are currently in the process of removing overloads from Phobos - not adding new ones - as the overloads are an implementation details, that the user of a generic function shouldn't be bothered with..

@ntrel: is there a chance you could maybe improve the text documentation so that it's even clearer?

ntrel · 2017-03-03T17:51:04Z

@wilzbach:

We are currently in the process of removing overloads from Phobos - not adding new ones - as the overloads are an implementation details, that the user of a generic function shouldn't be bothered with..

The 2 radix overloads are not implementation details, they are important for the user - all the other overloads only take one argument. I have now simplified the constraints (and removed one) so they only show what source type the radix overloads apply to. I have undocumented the static array ref (non-radix) overload as I agree that could be seen as an implementation detail.

@ntrel: is there a chance you could maybe improve the text documentation so that it's even clearer?

Possibly, but it would be fighting against ddoc's purpose of documenting declarations, we shouldn't be explaining what hidden arguments a function takes manually IMO. Documenting the 2 radix overloads normally we get the usual Parameters section etc.

I have also added 3 signatures manually at the top of the docs, partly because the actual signatures from this PR are so far below the pages of docs & examples, but also so the user can quickly grok the function, and the docs now refer to S, the source type from the signatures.

ntrel · 2017-03-13T14:49:54Z

@wilzbach Removed overloads from docs.

MetaLang

@ntrel needs a rebase.

MetaLang · 2017-07-21T18:21:04Z

std/conv.d

@@ -1732,7 +1735,7 @@ private T toImpl(T, S)(S value)
 /// ditto
 private T toImpl(T, S)(S value, uint radix)
    if (isInputRange!S && isSomeChar!(ElementEncodingType!S) &&
-        !isExactSomeString!T && is(typeof(parse!T(value, radix))))
+        isIntegral!T && is(typeof(parse!T(value, radix))))


What's with the template constraint change? We can't have that in a doc PR. Please remove it.

Moved to #5647.

ntrel · 2017-07-31T14:31:34Z

@wilzbach I can't work out why ./circleci.sh style_lint is failing on this. Any ideas?

wilzbach · 2017-07-31T18:49:31Z

@wilzbach I can't work out why ./circleci.sh style_lint is failing on this. Any ideas?

Not really, looked pretty strange to me as well. However, rebasing to the latest master commit worked :)

ntrel · 2017-08-01T11:24:31Z

@wilzbach Thanks!

quickfur · 2018-01-26T19:45:26Z

The changes look sensible to me. Improving docs is always welcome. Please rebase and lets get things moving again! ping @wilzbach @ntrel @MetaLang

Document overloads of `to` which take a radix.

Hide the static array ref overload from docs. Omit constraints involving T (unnecessary for the user).

dlang-bot · 2018-01-29T22:16:57Z

Thanks for your pull request, @ntrel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

quickfur · 2018-01-29T22:17:40Z

My impatience got the better of me, so I took it upon myself to rebase this. Let's see if it passes; if so, I'm going to pull the trigger.

quickfur · 2018-01-29T22:42:57Z

ping @JackStouffer
Is this PR OK, or are you still against this?

wilzbach

My impatience got the better of me, so I took it upon myself to rebase this. Let's see if it passes; if so, I'm going to pull the trigger.

I really really like your enthusiasm and the way you most PRs in the Phobos queue ahead, but in this case I would be careful here as AFAICT we haven't had a precedent of specifying "pseudo-overloads" in the plain-text documentation.

wilzbach · 2018-01-29T22:51:27Z

std/conv.d

@@ -176,8 +176,9 @@ class ConvOverflowException : ConvException
 }

 /**
-The `to` template converts a value from one type _to another.
-The source type is deduced and the target type must be specified, for example the
+$(D_CODE T $(DDOC_PSYMBOL _to)$(DDOC_TEMPLATE_PARAM_LIST (S))(S value);)


There's any precedent for this, is there?

wilzbach · 2018-01-29T22:53:26Z

std/conv.d

 *
- * _To work around this, you can specify a radix for conversions involving numbers.
+ * To work around this, you can specify a radix for conversions involving numbers:
+ * $(D_CODE T $(DDOC_PSYMBOL _to)$(DDOC_TEMPLATE_PARAM_LIST (S))(S value, uint radix);)


Looks a bit weird on the docs:

and again no precedent of manually specifying the overload in the plain-text docs.

wilzbach · 2018-01-29T22:54:37Z

std/conv.d

+ *             $(D_PARAM radix) must be a value from 2 _to 36.
+ *             $(D_PARAM value) is treated as a signed value only if $(D_PARAM radix) is 10.
+ *             The characters A through Z are used _to represent values 10 through 36.
+ *             $(D_PARAM letterCase) is the case _to use for non-decimal output characters.)))


Before:

After:

quickfur · 2018-01-29T23:29:50Z

@wilzbach
Hmph, you're right. I just saw that this PR was approved by another reviewer but was rotting in the queue pending a rebase, so I just rebased it myself; didn't look too closely at the actual change.

There was a case (not sure if there still is now) where the ddoc header contained a "pseudo" declaration, I don't remember exactly now, it may have been reduce or one of those doubly-templated functions, i.e., template reduce(Seeds...) { auto reduce(R)(R range, ...) ... } where ddoc produces rather ugly declarations, even though the function is intended to be used just like a regular template function. In any case, it's very ugly to introduce this kind of pseudo declaration instead of fixing ddoc or refactoring the code so that it will be formatted better automatically.

But yeah, it's probably a bad idea to merge this without a good reason.

Perhaps the solution here might be to document the 2-argument to overloads separately, then link to this from the 1-argument to docs in the section that talks about conversions to integers with a radix argument.

JackStouffer · 2018-01-30T15:21:04Z

Perhaps the solution here might be to document the 2-argument to overloads separately, then link to this from the 1-argument to docs in the section that talks about conversions to integers with a radix argument.

I'm still against splitting the to docs for the reasons listed above.

An easy solution is to add a Params: section to the template ddoc which is currently absent.

quickfur · 2018-01-31T00:01:00Z

@JackStouffer Now that I look at at again, not only the current docs is missing a Params:, the only thing that's documented is the outer template itself, not any of its inner functions. And a Params: without any documented function declarations is pretty useless.

Worse yet, there's no indication whatever about what possible combination of arguments are possible. I see that since the last time I looked, the number of possible arguments has been generalized to 1 possible radix to now an additional LetterCase, and the to template itself is now variadic, forwarding an arbitrary number of arguments to toImpl. There's probably a very good reason for this, but this is IMO currently very poorly-documented. I mean, I didn't even know LetterCase got added until I read the code just now. Maybe this PR isn't the right answer, but the documentation for to seriously needs some work here.

quickfur · 2018-01-31T00:07:08Z

P.S. I would say, from a doc reader's POV, the bare minimum for the docs would be:

A brief summary of all possible ways to call to, e.g.:

to!T(); // convert to T
to!T(radix); // convert to integral type T with specified radix
to!T(radix, LetterCase.upper); // convert to integral type T with specified radix and letter casing

Brief synopsis of what kind of conversions are possible (this information is already there but interspersed with numerous code example blocks, making a high-level understanding impossible)
How to integrate a user type with to so that it can support custom conversions.

RazvanN7 · 2021-10-22T09:44:59Z

Most of the changes in this PR have already been addressed.

ntrel force-pushed the to-docs branch from 7ff1a1b to 978f2e9 Compare November 14, 2016 14:09

ntrel force-pushed the to-docs branch 2 times, most recently from 7c9fc34 to e7822ef Compare November 25, 2016 15:41

wilzbach added the Needs Decision label Feb 21, 2017

dlang-bot added Needs Work stalled labels Jun 22, 2017

MetaLang requested changes Jul 21, 2017

View reviewed changes

ntrel force-pushed the to-docs branch from 050cae5 to 805581e Compare July 22, 2017 10:52

ntrel requested a review from JackStouffer as a code owner July 22, 2017 10:52

wilzbach force-pushed the to-docs branch from 805581e to d8ea035 Compare July 31, 2017 18:38

dlang-bot added Needs Rebase and removed Needs Rebase labels Jan 1, 2018

MetaLang approved these changes Jan 29, 2018

View reviewed changes

ntrel and others added 4 commits January 29, 2018 14:02

Improve docs for std.conv.to

68c14cd

Document overloads of `to` which take a radix.

Document basic function signatures for to

691eb76

Simplify to function constraints

7962a36

Hide the static array ref overload from docs. Omit constraints involving T (unnecessary for the user).

Move manual radix signatures to relevant places

4074790

Undocument to overloads to get other changes merged

a469694

quickfur force-pushed the to-docs branch from d8ea035 to a469694 Compare January 29, 2018 22:16

dlang-bot removed Needs Work stalled labels Jan 29, 2018

wilzbach reviewed Jan 29, 2018

View reviewed changes

dlang-bot added Needs Rebase Needs Work stalled labels May 27, 2021

RazvanN7 closed this Oct 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[docs] Improve docs for std.conv.to #4900

[docs] Improve docs for std.conv.to #4900

ntrel commented Nov 14, 2016 •

edited

JackStouffer commented Dec 6, 2016

ntrel commented Dec 15, 2016

wilzbach commented Feb 28, 2017

ntrel commented Mar 3, 2017

ntrel commented Mar 13, 2017

MetaLang left a comment

MetaLang Jul 21, 2017

ntrel Jul 22, 2017

ntrel commented Jul 31, 2017

wilzbach commented Jul 31, 2017

ntrel commented Aug 1, 2017

quickfur commented Jan 26, 2018

dlang-bot commented Jan 29, 2018

quickfur commented Jan 29, 2018

quickfur commented Jan 29, 2018 •

edited

wilzbach left a comment

wilzbach Jan 29, 2018

wilzbach Jan 29, 2018

wilzbach Jan 29, 2018

quickfur commented Jan 29, 2018

JackStouffer commented Jan 30, 2018

quickfur commented Jan 31, 2018

quickfur commented Jan 31, 2018

RazvanN7 commented Oct 22, 2021

[docs] Improve docs for std.conv.to #4900

[docs] Improve docs for std.conv.to #4900

Conversation

ntrel commented Nov 14, 2016 • edited

JackStouffer commented Dec 6, 2016

ntrel commented Dec 15, 2016

wilzbach commented Feb 28, 2017

ntrel commented Mar 3, 2017

ntrel commented Mar 13, 2017

MetaLang left a comment

Choose a reason for hiding this comment

MetaLang Jul 21, 2017

Choose a reason for hiding this comment

ntrel Jul 22, 2017

Choose a reason for hiding this comment

ntrel commented Jul 31, 2017

wilzbach commented Jul 31, 2017

ntrel commented Aug 1, 2017

quickfur commented Jan 26, 2018

dlang-bot commented Jan 29, 2018

Bugzilla references

quickfur commented Jan 29, 2018

quickfur commented Jan 29, 2018 • edited

wilzbach left a comment

Choose a reason for hiding this comment

wilzbach Jan 29, 2018

Choose a reason for hiding this comment

wilzbach Jan 29, 2018

Choose a reason for hiding this comment

wilzbach Jan 29, 2018

Choose a reason for hiding this comment

quickfur commented Jan 29, 2018

JackStouffer commented Jan 30, 2018

quickfur commented Jan 31, 2018

quickfur commented Jan 31, 2018

RazvanN7 commented Oct 22, 2021

ntrel commented Nov 14, 2016 •

edited

quickfur commented Jan 29, 2018 •

edited