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

Lint missing docs in icu_datetime #725

Merged
merged 5 commits into from
Aug 31, 2021
Merged

Conversation

gregtatum
Copy link
Member

This is an initial stab at #686 for icu_datetime. There are still lots of docs to write here.

@gregtatum gregtatum changed the title (wip) Missing docs (wip) Lint missing docs in icu_datetime May 17, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #725 (b6df5ad) into main (1f4a950) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #725      +/-   ##
==========================================
- Coverage   74.68%   74.66%   -0.03%     
==========================================
  Files         178      178              
  Lines       10710    10712       +2     
==========================================
- Hits         7999     7998       -1     
- Misses       2711     2714       +3     
Impacted Files Coverage Δ
components/datetime/src/date.rs 49.18% <0.00%> (-0.82%) ⬇️
components/datetime/src/lib.rs 100.00% <ø> (ø)
components/datetime/src/provider/time_zones.rs 18.18% <ø> (ø)
experimental/provider_ppucd/src/parse_ppucd.rs 92.99% <0.00%> (-0.14%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f4a950...b6df5ad. Read the comment docs.

@coveralls
Copy link

coveralls commented May 17, 2021

Pull Request Test Coverage Report for Build cce347fb872c76e3729b471c5a9707f5ad4398c5-PR-725

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 12 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.008%) to 75.018%

Files with Coverage Reduction New Missed Lines %
components/datetime/src/options/length.rs 1 25.0%
experimental/provider_ppucd/src/parse_ppucd.rs 1 93.28%
components/datetime/src/options/components.rs 4 77.39%
components/datetime/src/date.rs 6 50.0%
Totals Coverage Status
Change from base Build f52429e0ca3505eee821bd57218c20a80013283a: -0.008%
Covered Lines: 9585
Relevant Lines: 12777

💛 - Coveralls

@sffc
Copy link
Member

sffc commented Jul 29, 2021

I think we should merge this with the docs Greg wrote, even if they aren't comprehensive. It's a step in the right direction, and better to be merged than to sit in a PR.

@sffc sffc added the waiting-on-author PRs waiting for action from the author for >7 days label Jul 29, 2021
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • components/datetime/src/date.rs is different
  • components/datetime/src/datetime.rs is now changed in the branch
  • components/datetime/src/format/zoned_datetime.rs is now changed in the branch
  • components/datetime/src/lib.rs is different
  • components/datetime/src/options/components.rs is now changed in the branch
  • components/datetime/src/options/length.rs is now changed in the branch
  • components/datetime/src/options/preferences.rs is now changed in the branch
  • components/datetime/src/pattern/error.rs is now changed in the branch
  • components/datetime/src/provider/gregory.rs is now changed in the branch
  • components/datetime/src/provider/mod.rs is different
  • components/datetime/src/provider/time_zones.rs is different
  • components/datetime/src/skeleton.rs is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@gregtatum gregtatum marked this pull request as ready for review August 12, 2021 20:09
@gregtatum
Copy link
Member Author

I finished either filling in the docs or adding suppressions.

@gregtatum gregtatum changed the title (wip) Lint missing docs in icu_datetime Lint missing docs in icu_datetime Aug 12, 2021
@gregtatum gregtatum removed the waiting-on-author PRs waiting for action from the author for >7 days label Aug 12, 2021
nordzilla
nordzilla previously approved these changes Aug 12, 2021
Copy link
Member

@nordzilla nordzilla left a comment

Choose a reason for hiding this comment

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

LGTM, pending a few minor comments.

@@ -270,6 +288,7 @@ pub struct DayOfYearInfo {
/// assert_eq!(7, IsoWeekday::Sunday as usize);
/// ```
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
#[allow(missing_docs)] // The weekday variants should be self-obvious.
Copy link
Member

Choose a reason for hiding this comment

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

Question

This has a doc comment. Why is the linter saying it's missing?

Copy link
Member Author

Choose a reason for hiding this comment

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

All fields require doc comments. Here the comments wouldn't add any more meaning.

e.g.

/// ...
pub enum IsoWeekday {
    /// This is a Monday!
    Monday = 1,
    ....
}

components/datetime/src/date.rs Outdated Show resolved Hide resolved
pub mod key {
use icu_provider::{resource_key, ResourceKey};

/// A [`ResourceKey`](icu_provider::prelude::ResourceKey) to
/// [`gregory::DatesV1`](crate::provider::gregory::DatePatternsV1).
Copy link
Member

@nordzilla nordzilla Aug 12, 2021

Choose a reason for hiding this comment

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

Thought

I've been thinking a bit about our documentation style. Manually linking these keywords can be very verbose, tedious, and pain to maintain if the paths ever change.

Question

How do you feel about adding a doc-only use declaration to the top of the file?

e.g.

#[cfg(doc)]
use {
    crate::provider::{
        gregory::{DatePatternsV1, DateSymbolsV1},
        time_zones::{
            ExemplarCitiesV1, MetaZoneGenericNamesLongV1, MetaZoneGenericNamesShortV1,
            MetaZoneSpecificNamesLongV1, MetaZoneSpecificNamesShortV1, TimeZoneFormatsV1,                           
        },  
    },  
    icu_provider::prelude::ResourceKey,
};

Then your comment could just be

/// A [`ResourceKey`] to [`DatePatternsV1`].

We would only need to re-link it if we wanted to explicitly rename it, and even then there are no paths to deal with, so it still looks clean:

/// A [`ResourceKey`] to [`DatesV1`](DatePatternsV1).

I think this style would really unclutter our documentation. I think it's nice to have all the doc-only dependencies in one spot. I think it's also nice that it gives readers of the file some intuition of related modules, even though they aren't directly included as dependencies when building the code itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah that would be great, and they should hopefully be checked by the compiler.

zbraniecki
zbraniecki previously approved these changes Aug 26, 2021
@sffc sffc added the waiting-on-author PRs waiting for action from the author for >7 days label Aug 26, 2021
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • components/datetime/src/date.rs is different
  • components/datetime/src/provider/mod.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@gregtatum gregtatum removed the waiting-on-author PRs waiting for action from the author for >7 days label Aug 30, 2021
Copy link
Contributor

@dminor dminor left a comment

Choose a reason for hiding this comment

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

lgtm!

@gregtatum gregtatum merged commit 26b1be1 into unicode-org:main Aug 31, 2021
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.

7 participants