-
-
Notifications
You must be signed in to change notification settings - Fork 725
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rearrange and extend string documentation #2997
Conversation
Do you have a screenshot of the resulting documentation? |
Good point: http://imgur.com/AKwpvCX |
$(XREF uni, toUpperInPlace) | ||
)) | ||
) | ||
|
||
Not all functions for D _string handling are in this module. Functions related |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better if this paragraph came before the above table? It reads a little out-of-order right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it? I meant to go from local to global: local functions, publicly imported functions, other modules. Assuming someone that looks into the string module might want to know what is there first.
I am happy to change this, of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see what you mean. I just thought it was a bit awkward that we have already listed publicly imported symbols, which are obviously from elsewhere, and then repeat ourselves that not every symbol is defined in this module. But you do have a good point. Perhaps an even better way is to change this sentence to read something like "There are other string handling functions that are defined in other modules: ...".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like you positive attitude. That is, not starting the sentence with not
.
7a8b7f0
to
7433030
Compare
@WalterBright The new |
Yes it should be. Yet another PR defect that slipped through. Where, oh where, is our contributors'/reviewers' guide when we need it... |
LGTM. Thanks! |
wider generality than just strings can be found in $(LINK2 std_algorithm.html, | ||
std.algorithm) and $(LINK2 std_range.html, std.range). | ||
|
||
See_Also: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't introduced by this PR, but for some reason, this section heading is showing up as SeeAlso
(one word) instead of See Also
(two words). Seems to be the artifact of some kind of postprocessing script in the dlang.org build. There are several places where such multi-word headings are used; we better fix them before next release...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't mean fix it in this PR, but in whatever's causing this artifact.
Nice. Perhaps the publicly imported modules table could drop the repeated module name prefixes for better readability. The module column should then list |
1861e86
to
008d08f
Compare
Good thinking. The external links did not look nice. I meant to use the "official" macros, but it certainly looks better this way. |
Display function name for publicly imported functions only. Use $(TT ...) formating to match "normal" function table. Add leading row description
008d08f
to
55fcbc8
Compare
Looks better, but I'm not sure about the "std.uni comparison functions", etc., links. It's two words instead of one, which is confusing because everything else in the right-hand column are single-word links. Also, the links themselves just point to the generic module documentation rather than specifically "comparison functions", etc.. I think these links should be pulled out of the table and put somewhere else, something like "see also these modules: ...". |
Ditto with the "std.algorithm search functions", which is actually an outdated link since it should point to |
I must confess, I am somewhat lost with your remarks. Are you sure you are looking at the latest version? Here is what it looks like on my computer: http://i.imgur.com/jCSt0iP.png?1 |
Hmph, I must be looking at an outdated version. Sorry about that. I'll pull again and update. |
OK, looks really good now. I should've checked I actually have the latest version of your code before commenting. My apologies... Anyway, LGTM. |
Auto-merge toggled on |
Thanks a lot for the reviews. |
Rearrange and extend string documentation
List the locally defined functions and public imported functions in different tables. Move links to other modules to
see also
section. More pleasant to the eye.