Skip to content
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

Merged
merged 3 commits into from
Feb 24, 2015

Conversation

kuettler
Copy link
Contributor

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.

@quickfur
Copy link
Member

Do you have a screenshot of the resulting documentation?

@kuettler
Copy link
Contributor Author

Good point: http://imgur.com/AKwpvCX

$(XREF uni, toUpperInPlace)
))
)

Not all functions for D _string handling are in this module. Functions related
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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: ...".

Copy link
Contributor Author

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.

@kuettler
Copy link
Contributor Author

@WalterBright The new splitterLines function is not mentioned in the top-level functions table yet. Should it be there?

@quickfur
Copy link
Member

Yes it should be. Yet another PR defect that slipped through. Where, oh where, is our contributors'/reviewers' guide when we need it...

@quickfur
Copy link
Member

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:
Copy link
Member

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...

Copy link
Member

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.

@ntrel
Copy link
Contributor

ntrel commented Feb 20, 2015

Nice. Perhaps the publicly imported modules table could drop the repeated module name prefixes for better readability. The module column should then list std.algorithm rather than just algorithm, etc.

@kuettler
Copy link
Contributor Author

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
@quickfur
Copy link
Member

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: ...".

@quickfur
Copy link
Member

Ditto with the "std.algorithm search functions", which is actually an outdated link since it should point to std.algorithm.searching now. But still, it's a link to an entire module's docs as opposed to jumping to a specific symbol, so I would pull all of these links out of the table and put them in a separate list of other modules of interest.

@kuettler
Copy link
Contributor Author

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

@quickfur
Copy link
Member

Hmph, I must be looking at an outdated version. Sorry about that. I'll pull again and update.

@quickfur
Copy link
Member

OK, looks really good now. I should've checked I actually have the latest version of your code before commenting. My apologies...

Anyway, LGTM.

@quickfur
Copy link
Member

Auto-merge toggled on

@kuettler
Copy link
Contributor Author

Thanks a lot for the reviews.

braddr added a commit that referenced this pull request Feb 24, 2015
Rearrange and extend string documentation
@braddr braddr merged commit dd79748 into dlang:master Feb 24, 2015
@kuettler kuettler deleted the string_doc_cleanup branch February 24, 2015 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants