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

Add more docs. #366

Merged
merged 1 commit into from
Aug 22, 2024
Merged

Add more docs. #366

merged 1 commit into from
Aug 22, 2024

Conversation

waywardmonkeys
Copy link
Contributor

No description provided.

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

Lovely, thanks!

There is one nascent bug which this has made obvious but doesn't need to fix, but which does need addressing.

src/affine.rs Outdated Show resolved Hide resolved
///
/// Equivalent to `self * Affine::rotate_about(th)`
///
/// [rotation]: Affine::rotate_about
#[inline]
#[must_use]
pub fn pre_rotate_about(self, th: f64, center: Point) -> Self {
Affine::rotate_about(th, center) * self
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs to be:

Suggested change
Affine::rotate_about(th, center) * self
self * Affine::rotate_about(th, center)

Although that might be for a different PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs to be a separate PR. I think the core is wrong here, not the doc comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I meant was that I too think the code is wrong and this is a docs patch, so I'd rather handle it separately. :)

src/arc.rs Outdated Show resolved Hide resolved
Comment on lines +189 to 190
/// with `width` and `height` [rounded towards] zero to the nearest integer,
/// unless they are already an integer.
Copy link
Member

Choose a reason for hiding this comment

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

An optional suggestion - I'd slightly prefer this to be something like:

Suggested change
/// with `width` and `height` [rounded towards] zero to the nearest integer,
/// unless they are already an integer.
/// with `width` and `height` [truncated][rounded towards] to their integer part.
/// This effectively rounds them to the nearest integer towards zero.

I don't think "rounds towards zero to the nearest integer" flows very well, because in my mind "towards zero" is a modifier on the entire "rounds to the nearest integer".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather this one be addressed separately as this sort of text is in more than one place.

@waywardmonkeys waywardmonkeys added this pull request to the merge queue Aug 22, 2024
Merged via the queue into linebender:main with commit 65c6eb3 Aug 22, 2024
15 checks passed
@waywardmonkeys waywardmonkeys deleted the more-docs branch August 22, 2024 04:03
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.

2 participants