Skip to content

Conversation

@GrizzlT
Copy link
Member

@GrizzlT GrizzlT commented Oct 8, 2022

Apart from the derive macros, all items should be documented. Since this is the crate that does not depend on any other falcon crate, this should be understandable on its own.

Is that the case, is this clear and sufficient documentation?

@GrizzlT GrizzlT marked this pull request as ready for review October 10, 2022 17:05
Copy link
Collaborator

@VixieTSQ VixieTSQ left a comment

Choose a reason for hiding this comment

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

These are all nitpicks and absolutely not blockers. But what we discussed before is still a problem. I could not get a solid grasp on what the "seed-flavored" traits are really meant for and the external input stuff needs to be explained better.

Comment on lines +5 to +6
/// Helper type to write iterators over implementors of the seed-less flavored
/// traits to a minecraft connection.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does it mean to write iterators to a Minecraft connection? As a Bytes? Each individually?

Copy link
Member Author

Choose a reason for hiding this comment

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

Iterators over PacketWrite, meaning iterators over more complex types. U should use the specialized implementations for byte arrays.

Comment on lines +19 to +21
#[doc = "Transparent wrapper around an "]
#[doc = stringify!($base)]
#[doc = "."]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#[doc = "Transparent wrapper around an "]
#[doc = stringify!($base)]
#[doc = "."]
#[doc = concat!("Transparent wrapper around an ", stringify!($base), "."]

This might be more readable

}

impl $var {
/// Return the stored value.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Return the stored value.
/// Return the inner value.

Sounds more rusty

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