improve docs #15

robjtede · 2023-03-02T01:01:25Z

Improves docs on a large portion of methods on IpCidr, Ipv4Cidr, and Ipv6Cidr. Tries to adhere to standard rustdoc grammar and formatting guidelines including using imperative mood for first line of function docs.

Also includes some trivial code quality improvements. No changes to public API have been made.

magiclen · 2023-04-05T15:05:12Z

README.md

-assert_eq!(true, cidr.contains(IpAddr::from_str("192.168.51.103").unwrap()));
-assert_eq!(false, cidr.contains(IpAddr::from_str("192.168.50.103").unwrap()));
+assert_eq!(true, cidr.contains(IpAddr::from([192, 168, 51, 103])));
+assert_eq!(false, cidr.contains(IpAddr::from([192, 168, 50, 103])));
 ```


Why not use from_str?

examples should be clean and concise

removed this particular change from this PR

magiclen · 2023-04-05T15:07:11Z

README.md

@@ -160,7 +160,7 @@ Enable the `serde` feature to support the serde framework.

 ```toml
 [dependencies.cidr-utils]
-version = "*"


I prefer using *. Dot-dot-dot is not in correct format.

Encouraging use of * is dangerous. Using the "wrong" format requires users to select the version properly.

Sometimes I just want to copy-paste and run, and see what happens. I don't want to make an extra effort to correct the format.

My experience with helping Rust newcomers is that this is a real footgun. Regardless, I've dropped this commit.

magiclen · 2023-04-05T15:10:18Z

src/cidr/ip_cidr.rs

@@ -17,6 +17,7 @@ pub enum IpCidr {

 impl IpCidr {
    #[allow(clippy::should_implement_trait)]
+    #[doc(hidden)] // TODO: remove and rely on std FromStr


Is this function necessary to be hidden?

We should encourage use of the FromStr trait. If a breaking change were acceptable, I'd say to make this fn private. Hiding it is an alternative that is backwards compatible.

It is more convenient that a type has from_str itself. We don't need to add the use std::str::FromStr statement every time.

Using str::parse() is the recommended method, not FromStr, which is why it's not in the std prelude.

removed this particular change from this PR

magiclen · 2023-04-05T15:16:56Z

src/cidr/ip_cidr_iterators.rs

@@ -62,6 +62,8 @@ impl DoubleEndedIterator for IpCidrIpAddrIterator {
 }

 impl IpCidr {
+    #[doc(hidden)]
+    #[deprecated(note = "Prefer `.iter()`.")]


The reason why this function is named iter_as ip_addr is to be consistent with Ipv4Cidr::iter_as_ipv4_addr and Ipv6Cidr::iter_as_ipv6_addr.

fair enough, reverted

magiclen · 2023-04-05T15:19:40Z

src/cidr/v4/ipv4_cidr.rs

@@ -28,8 +28,9 @@ pub struct Ipv4Cidr {
 }

 impl Ipv4Cidr {
+    /// Returns an integer which represents the prefix an IPv4 byte array of this CIDR in big-endian
+    /// (BE) order.


I prefer not to wrap comments.

You don't want them wrapped at all, even for extremely long lines?

If the comment line is extremely long, I will wrap it by myself instead of tools,

up to you, I unwrapped these long lines

magiclen · 2023-04-05T15:21:42Z

src/cidr/v4/ipv4_cidr.rs

    }

    #[inline]
    pub fn get_bits(&self) -> u8 {
        mask_to_bits(self.mask).unwrap()
    }

+    /// Returns an integer which represents the mask an IPv4 byte array of this CIDR in big-endian
+    /// (BE) order.


I prefer not to wrap comments.

magiclen · 2023-04-05T15:22:31Z

src/cidr/v4/ipv4_cidr.rs

@@ -111,6 +109,7 @@ impl Ipv4Cidr {
    }

    #[allow(clippy::should_implement_trait)]
+    #[doc(hidden)] // TODO: remove and rely on std FromStr


Is this function necessary to be hidden?

removed this particular change from this PR

magiclen · 2023-04-05T15:23:54Z

src/cidr/v4/ipv4_cidr.rs

@@ -229,7 +243,7 @@ impl Ipv4Cidr {

 impl Debug for Ipv4Cidr {
    #[inline]
-    fn fmt(&self, f: &mut Formatter) -> Result<(), fmt::Error> {
+    fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), fmt::Error> {


Why we need to add <'_> explicitly?

It's part of the rust_2018_idioms lint, omitting this anon lifetime will be a hard error in the future.

magiclen · 2023-04-05T15:25:03Z

src/cidr/v6/ipv6_cidr.rs

@@ -123,6 +119,7 @@ impl Ipv6Cidr {
    }

    #[allow(clippy::should_implement_trait)]
+    #[doc(hidden)] // TODO: remove and rely on std FromStr


Is this function necessary to be hidden?

removed this particular change from this PR

magiclen · 2023-04-05T15:26:14Z

src/lib.rs

@@ -49,8 +49,8 @@ use cidr_utils::cidr::IpCidr;

 let cidr = IpCidr::from_str("192.168.51.0/24").unwrap();

-assert_eq!(true, cidr.contains(IpAddr::from_str("192.168.51.103").unwrap()));
-assert_eq!(false, cidr.contains(IpAddr::from_str("192.168.50.103").unwrap()));


Why not use from_str?

removed this particular change from this PR

magiclen · 2023-04-05T15:26:44Z

src/lib.rs

@@ -158,12 +158,16 @@ Enable the `serde` feature to support the serde framework.

 ```toml
 [dependencies.cidr-utils]
-version = "*"
+version = "..."


I prefer using *. Dot-dot-dot is not in correct format.

removed this particular change from this PR

magiclen · 2023-04-05T15:29:07Z

src/lib.rs

+#![deny(rust_2018_idioms, nonstandard_style)]
+#![warn(future_incompatible)]
+#![cfg_attr(docsrs, feature(doc_auto_cfg))]
+


Why we need to add these?

They provide good defaults for contributors and encourage better code style.

Why fn fmt(&self, f: &mut Formatter<'_>) is better than fn fmt(&self, f: &mut Formatter)?

Because hidden lifetimes are harmful to comprehension. These lints are likely to become errors in a future edition.

robjtede marked this pull request as draft March 2, 2023 01:01

robjtede force-pushed the improve-docs branch from 6f838df to 5fe5cec Compare March 2, 2023 01:08

robjtede force-pushed the improve-docs branch from 5fe5cec to c392714 Compare March 30, 2023 02:48

robjtede marked this pull request as ready for review March 30, 2023 02:48

magiclen reviewed Apr 5, 2023

View reviewed changes

robjtede force-pushed the improve-docs branch from c392714 to b107976 Compare September 12, 2023 05:06

add some top-level lints

108cdb4

robjtede force-pushed the improve-docs branch 3 times, most recently from 95a3389 to f7adf2d Compare September 12, 2023 05:16

improve docs on Cidr* structs

fadab2f

robjtede force-pushed the improve-docs branch from f7adf2d to fadab2f Compare September 12, 2023 05:19

robjtede closed this Dec 27, 2023

robjtede deleted the improve-docs branch December 27, 2023 00:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve docs #15

improve docs #15

robjtede commented Mar 2, 2023 •

edited

magiclen Apr 5, 2023

robjtede Apr 5, 2023

robjtede Sep 12, 2023

magiclen Apr 5, 2023 •

edited

robjtede Apr 5, 2023 •

edited

magiclen Apr 14, 2023

robjtede Sep 12, 2023

magiclen Apr 5, 2023

robjtede Apr 5, 2023

magiclen Apr 14, 2023

robjtede Jun 29, 2023 •

edited

robjtede Sep 12, 2023

magiclen Apr 5, 2023

robjtede Sep 12, 2023

magiclen Apr 5, 2023

robjtede Apr 11, 2023

magiclen Apr 14, 2023

robjtede Sep 12, 2023

magiclen Apr 5, 2023

magiclen Apr 5, 2023

robjtede Sep 12, 2023

magiclen Apr 5, 2023

robjtede Jun 29, 2023 •

edited

magiclen Apr 5, 2023

robjtede Sep 12, 2023

magiclen Apr 5, 2023

robjtede Sep 12, 2023

magiclen Apr 5, 2023 •

edited

robjtede Sep 12, 2023

magiclen Apr 5, 2023

robjtede Apr 5, 2023 •

edited

magiclen Apr 14, 2023

robjtede Sep 12, 2023

improve docs #15

improve docs #15

Conversation

robjtede commented Mar 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magiclen Apr 5, 2023 • edited

Choose a reason for hiding this comment

robjtede Apr 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robjtede Jun 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robjtede Jun 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magiclen Apr 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robjtede Apr 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robjtede commented Mar 2, 2023 •

edited

magiclen Apr 5, 2023 •

edited

robjtede Apr 5, 2023 •

edited

robjtede Jun 29, 2023 •

edited

robjtede Jun 29, 2023 •

edited

magiclen Apr 5, 2023 •

edited

robjtede Apr 5, 2023 •

edited