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

Prepare 0.10.4 release; DashIterator doc #319

Merged
merged 2 commits into from
Oct 20, 2023
Merged

Prepare 0.10.4 release; DashIterator doc #319

merged 2 commits into from
Oct 20, 2023

Conversation

raphlinus
Copy link
Contributor

Improve documentation of DashIterator and bump version in preparation for release.

Improve documentation of DashIterator and bump version in preparation for release.
Copy link
Contributor

@dfrg dfrg left a comment

Choose a reason for hiding this comment

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

Just bikeshedding a bit: I wasn’t aware this was already public. The API incongruity (with segments and maybe flatten) is a bit unfortunate. I would have preferred fn dash(…) -> impl Iterator (or -> Dash if following Rust naming guidelines and matching Segments if we want the concrete type exposed).

src/stroke.rs Outdated
@@ -613,6 +613,18 @@ const DASH_ACCURACY: f64 = 1e-6;

impl<'a, T: Iterator<Item = PathEl>> DashIterator<'a, T> {
/// Create a new dashing iterator.
///
/// Handling of dashes is fairly orthogonal to stroke expansion. This iterator
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this comment should be attached to the type instead?

@raphlinus
Copy link
Contributor Author

I'm ok with changing it to look like a function. That should probably be a bump to 0.11, as it would certainly be semver breaking, unless we left it in as an alias with doc(hidden) maybe.

If it were changed to a function, the main comment should be attached to that, not the type, yes?

@dfrg
Copy link
Contributor

dfrg commented Oct 20, 2023

I was thinking the same thing wrt doc(hidden) :)

And yes, the lovely new doc comment should be on the function in this case.

As per review comments
@raphlinus raphlinus merged commit 38bc948 into main Oct 20, 2023
14 checks passed
@raphlinus raphlinus deleted the dashiter branch October 20, 2023 16:30
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