-
Notifications
You must be signed in to change notification settings - Fork 61
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
Conversation
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... |
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. |
Just to be clear, I'm talking about your first comment in this thread, where you mentioned |
Oh, sorry I didn't realize the PR title and description ended up in the merge commit. Almost hit push here. |
Is this good? |
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... |
I just took the liberty to remove it from the title, so it's not double. |
bors r+ |
Build succeeded |
gradient.take(n).rev()
is now possible, closes #153