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

Update public API docs #197

Merged
merged 41 commits into from
Nov 27, 2021
Merged

Update public API docs #197

merged 41 commits into from
Nov 27, 2021

Conversation

chrissimpkins
Copy link
Collaborator

@chrissimpkins chrissimpkins commented Nov 8, 2021

Adds the #![warn(missing_docs)] attribute and full public API documentation

Related #195

@chrissimpkins chrissimpkins mentioned this pull request Nov 8, 2021
10 tasks
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments inline that you are free to ignore :)

src/data_request.rs Outdated Show resolved Hide resolved
src/data_request.rs Outdated Show resolved Hide resolved
src/data_request.rs Outdated Show resolved Hide resolved
Comment on lines 72 to 74
/// Returns a mutable reference to a [DataRequest] that includes
/// glyph layers and points if `b` is `true` or excludes these
/// data if `b` is `false`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rust doc comments work a lot like git commits: the first line is used as a short summary (and is shown beside an item in the module level docs), and then anything after the first newline is "additional detail" and is only shown on the docs page for that specific item.

So basically: one short line that is a summary, and then anything else below the fold.

In this case though, I'm not sure that the new docs are an improvement on the previous ones? In particular I don't think it is necessary to mention that this returns &mut Self. This pattern should be familiar to the caller from std (see std::process::Command). If the previous docstring was too vague, I might instead say,

/// Specify whether layers (and glyphs) should be loaded.

Copy link
Collaborator Author

@chrissimpkins chrissimpkins Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted. I will have a look into this one tonight and respond.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this information Colin. I agree with your comments here and revised the initial draft in fb78ef7

Please let me know if you feel that this improves the documentation in the section.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related:

I've tended towards adding a # Notes header in documentation sections that have longer and/or multi-issue text content below the summary line. IMO this sets it off nicely in the HTML site, but this is very subjective. If you feel that this leads to unnecessary header clutter in the docs please let me know. I should have a full first draft available by the end of the day so that we can have a look.

Copy link
Member

@cmyr cmyr Nov 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that std is a great guideline, and we should try to match the conventions used there. They don't seem to use # Note as a header very often, but they do often have lines that begin with 'Note:', for providing information that is not important in the general case, but is important to a deeper understanding of the API: see for instance the docs for String::drain.

I do think I've seen # Note: before, but generally for a longform exposition of some idea or concept, but I can't find an example. The best I can come up with is the A ghastly note section of PhantomData.

Copy link
Collaborator Author

@chrissimpkins chrissimpkins Nov 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tyvm! I'll do another pass through all of the new docs and remove unnecessary Note headers that do not follow this guideline.

Edit: 314e216 and 6c55831 address this issue

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how it appears in the HTML

2021-11-10_13-23-59

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vs. this after 314e216

2021-11-10_13-28-56

src/datastore.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
@chrissimpkins
Copy link
Collaborator Author

Some comments inline that you are free to ignore :)

Ty!!

@chrissimpkins
Copy link
Collaborator Author

font module doc updates in 1462bca

@chrissimpkins
Copy link
Collaborator Author

chrissimpkins commented Nov 9, 2021

fontinfo module docs e038f7a

groups module docs 064d186

guideline module docs 4b59dbd

kerning module docs 1467c19

layer module docs d745027

shared_types module docs 5b6080d

write module docs 2c9325f

glyph module docs 01cc4e7

glyph::serialize docs 30a30a8

@chrissimpkins
Copy link
Collaborator Author

chrissimpkins commented Nov 10, 2021

At this stage, I am simply trying to get a first draft of documentation in place across everything that is public. We're nearly there. There are 38 missing public X documentation warnings left as of 2c9325f, down from nearly 400.

The next step will be another pass for revisions with additional examples where appropriate and confirmation that the new content is valid/accurate, concise, and properly formatted in the production HTML site gen'd with cargo doc. Part of this effort will be to confirm that we use the proper rustdoc format as Colin noted in #197 (comment). I'll catch outstanding formatting issues that were not revised in the first draft on the next pass.

Please don't hesitate to edit this branch directly if you come across anything that should be changed.

@chrissimpkins chrissimpkins marked this pull request as ready for review November 10, 2021 18:07
@chrissimpkins
Copy link
Collaborator Author

chrissimpkins commented Nov 10, 2021

As of 30a30a8, the entire public API is documented and clippy should no longer raise errors. Converted out of Draft status. This is ready for review. I will continue to work on the revisions noted in #197 (comment) and #197 (comment)

@chrissimpkins chrissimpkins changed the title Update docs Update public API docs Nov 10, 2021
@chrissimpkins
Copy link
Collaborator Author

Rebased on #201

@madig
Copy link
Collaborator

madig commented Nov 22, 2021

Question about the breadth of the docs: In e.g. a Glyph, the name should not contain control characters and in the list of Unicode codepoints, the first one is the primary codepoint for the glyph (see https://unifiedfontobject.org/versions/ufo3/glyphs/glif/). Should this information be included in the norad docs?

@chrissimpkins
Copy link
Collaborator Author

IMO, yes. I am a fan of comprehensive docs.

@madig madig added this to the 1.0.0 milestone Nov 23, 2021
@chrissimpkins
Copy link
Collaborator Author

Should we merge this before any significant source changes land? I can keep an eye out and rebase / edit in this branch if you'd prefer to keep this open for now.

@madig
Copy link
Collaborator

madig commented Nov 26, 2021

I'd like it merged so we have less work with subsequent PRs :) @cmyr ?

@cmyr
Copy link
Member

cmyr commented Nov 26, 2021

Sure let's get this in and we can always touch it up later :)

@madig madig merged commit ddaaaf6 into linebender:master Nov 27, 2021
@chrissimpkins chrissimpkins deleted the update-docs branch November 27, 2021 01:43
@chrissimpkins
Copy link
Collaborator Author

Thank you both very much for the reviews!

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.

3 participants