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

Documentation tweaks #1274

Merged
merged 7 commits into from
Sep 27, 2023
Merged

Conversation

pitdicker
Copy link
Collaborator

@pitdicker pitdicker commented Sep 8, 2023

This is one of those things that have been sitting as a half-finished branch for a couple of months...

I would like to tweak the inline/no-inline attributes of re-exports in lib.rs so that the the useful types are better visible than the ones that are rarely needed return types.

Before

afbeelding

After

afbeelding

@pitdicker pitdicker force-pushed the documentation_tweaks branch from b53ea5e to e5f98a1 Compare September 8, 2023 18:15
@pitdicker pitdicker marked this pull request as draft September 8, 2023 18:19
@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Merging #1274 (77bb121) into 0.4.x (22846c9) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##            0.4.x    #1274   +/-   ##
=======================================
  Coverage   91.50%   91.50%           
=======================================
  Files          38       38           
  Lines       17313    17314    +1     
=======================================
+ Hits        15843    15844    +1     
  Misses       1470     1470           
Files Coverage Δ
src/datetime/mod.rs 89.29% <ø> (+0.14%) ⬆️
src/datetime/rustc_serialize.rs 92.72% <ø> (ø)
src/datetime/serde.rs 87.56% <ø> (ø)
src/format/mod.rs 85.04% <ø> (ø)
src/lib.rs 95.58% <ø> (ø)
src/naive/date.rs 96.22% <ø> (ø)
src/naive/datetime/mod.rs 97.63% <ø> (ø)
src/naive/time/mod.rs 96.53% <ø> (ø)
src/offset/local/mod.rs 85.32% <ø> (ø)
src/offset/mod.rs 56.00% <ø> (ø)
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pitdicker pitdicker force-pushed the documentation_tweaks branch 4 times, most recently from d38da02 to 3ea9c0f Compare September 8, 2023 19:07
@pitdicker
Copy link
Collaborator Author

This took a little more effort.

If you don't document a type right at the crate root, it must be in another public module or otherwise it will not be documented:

  • I made the round module public, so we can hide the DurationRound trait and co. in there (I don't like that one all 😄).
  • SecondsFormat is moved from the private datetime module to public module formatting. Seems like a better place anyway.

And because types such as NaiveDate and NaiveTime now have a copy of their documentation in the main module, old manual links in their methods were broken. I updated them to rust paths (but did not go over all documentation to fix all old-style links, just the ones that were broken).

@pitdicker pitdicker marked this pull request as ready for review September 8, 2023 19:15
@pitdicker pitdicker force-pushed the documentation_tweaks branch 3 times, most recently from 4f07c82 to b4bfa59 Compare September 24, 2023 06:01
@@ -533,7 +533,7 @@ pub use offset::LocalResult;
#[doc(inline)]
pub use offset::{FixedOffset, Offset, TimeZone, Utc};

mod round;
pub mod round;
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the argument put forth in the commit message. What's the rationale for this change?

Copy link
Collaborator Author

@pitdicker pitdicker Sep 24, 2023

Choose a reason for hiding this comment

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

The idea is to highlight the importants traits in the "Traits" section: TimeZone, Offset, Datelike and Timelike.
I don't think DurationRound and SubsecRound are good enough to be part of the list in the main documentation.
We can only hide it in the list if the round module is public.

To say it stronger, in my opinion the DurationRound trait is something that begs for a replacement.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, sure, but if that's the goal IMO the other changes to that goal should be in the same commit as that. In general, the first commit here seems too large/unfocused (it probably doesn't help that I'm completely unfamiliar with the semantics of doc(no_inline))...

Copy link
Collaborator Author

@pitdicker pitdicker Sep 25, 2023

Choose a reason for hiding this comment

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

O, sorry. That was only the goal of that commit. No: the goal of the PR was tweaking doc(inline) and doc(no_inline).

If the original module is public:

  • a re-export comes under the list "Re-exports".
  • doc(inline) on a re-export copies the documentation, so it now lives at two places.

If the original module is private:

  • a re-export moves the documentation.
  • doc(no_inline) makes the documentation unreachable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In general, the first commit here seems too large/unfocused

I have split it up into three commits.

src/lib.rs Outdated

mod round;
pub mod round;
#[doc(no_inline)]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, that means this line does nothing 😄.

@pitdicker pitdicker force-pushed the documentation_tweaks branch 2 times, most recently from d96eb11 to 9210e0a Compare September 25, 2023 11:47
@pitdicker pitdicker merged commit 604b92a into chronotope:0.4.x Sep 27, 2023
36 of 37 checks passed
@pitdicker pitdicker deleted the documentation_tweaks branch September 27, 2023 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants