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 DoubleEndedIterator impl for gradient::Take #154

Merged
merged 1 commit into from Nov 26, 2019

Conversation

jansegre
Copy link
Contributor

@jansegre jansegre commented Nov 26, 2019

gradient.take(n).rev() is now possible, closes #153

@Ogeon
Copy link
Owner

Ogeon commented Nov 26, 2019

Looks like a fine solution for preserving the current behavior. 👍 An alternative could have been to use start position, end position and step size, but I don't know if it improves anything.

The skew towards the beginning of the range should also be fixed at some point, as I mentioned, but it may be better to do it in a separate PR. It's technically a breaking change, while this doesn't have to be.

The formatting is fine, but you can always just stage and commit the file you have changed, and reset the rest if you want. Either way, I would appreciate it a lot if you can write a short description and include the "closes ..." part in the PR description/initial comment. That makes it show up nicely in the merge commit. 🙂 I should really add a PR template for this...

@jansegre
Copy link
Contributor Author

jansegre commented Nov 26, 2019

OK, I added the "closes #issue" to the title. But I guess it's better on the commit message body alongside a short description. I'll do that and amend this commit.

@Ogeon
Copy link
Owner

Ogeon commented Nov 26, 2019

Just to be clear, I'm talking about your first comment in this thread, where you mentioned cargo fmt. That goes into the merge commit.

@jansegre
Copy link
Contributor Author

Oh, sorry I didn't realize the PR title and description ended up in the merge commit. Almost hit push here.

@jansegre
Copy link
Contributor Author

Is this good?

@Ogeon
Copy link
Owner

Ogeon commented Nov 26, 2019

That's excellent, thanks! It's not easy to know, but quite nice IMO. I just need to make that template, so it's more obvious...

@Ogeon Ogeon changed the title Add DoubleEndedIterator impl for gradient::Take, closes #153 Add DoubleEndedIterator impl for gradient::Take Nov 26, 2019
@Ogeon
Copy link
Owner

Ogeon commented Nov 26, 2019

I just took the liberty to remove it from the title, so it's not double.

@Ogeon
Copy link
Owner

Ogeon commented Nov 26, 2019

bors r+

bors bot added a commit that referenced this pull request Nov 26, 2019
154: Add DoubleEndedIterator impl for gradient::Take r=Ogeon a=jansegre

`gradient.take(n).rev()` is now possible, closes #153

Co-authored-by: Jan Segre <jan@segre.in>
@bors
Copy link
Contributor

bors bot commented Nov 26, 2019

Build succeeded

@bors bors bot merged commit 902150c into Ogeon:master Nov 26, 2019
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.

impl std::iter::DoubleEndedIterator for palette::gradient::Take
2 participants