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

change revRange to same argument types as range #374

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

borovan
Copy link

@borovan borovan commented May 6, 2022

This allows it to iterate returning ?Nat instead of ?Int so it can be used in arrays.
As range starts at Nat I figured that negative numbers weren't that important anyway. Maybe range should be (Nat, Nat) too? So then you've got the two functions that are easily used with arrays. Could always add rangeInt separately.

This is my first pull request so if I've screwed anything up DONT HOLD BACK!

Here are the tests I used

// revRange
do {
let rev = LIter.revRange(0, 0);
assert(rev.next() == ?0);
assert(rev.next() == null);
};
do {
let rev = LIter.revRange(3, 1);
assert(rev.next() == ?3);
assert(rev.next() == ?2);
assert(rev.next() == ?1);
assert(rev.next() == null);
};

This allows it to iterate returning ?Nat instead of ?Int so it can be used in arrays.
As range starts at Nat I figured that negative numbers weren't that important anyway. Maybe range should be (Nat, Nat) too? So then you've got the two functions that are easily used with arrays. Could always add rangeInt separately.

This is my first pull request so if I've screwed anything up DONT HOLD BACK!

Here are the tests I used

// revRange
do {
  let rev = LIter.revRange(0, 0);
  assert(rev.next() == ?0);
  assert(rev.next() == null);
};
do {
  let rev = LIter.revRange(3, 1);
  assert(rev.next() == ?3);
  assert(rev.next() == ?2);
  assert(rev.next() == ?1);
  assert(rev.next() == null);
};
@ghost
Copy link

ghost commented May 6, 2022

Dear @borovan,

In order to potentially merge your code in this open-source repository and therefore proceed with your contribution, we need to have your approval on DFINITY's CLA1.

If you decide to agree with it, please visit this issue and read the instructions there.

— The DFINITY Foundation

Footnotes

  1. Contributor License Agreement

Copy link
Contributor

@nomeata nomeata left a comment

Choose a reason for hiding this comment

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

I think the original plan was to produce a Nat iterator, and the only reason to accept an Int in the bound is so that you can write something like a.len() - 1 without worrying about underflow. But that design isn't beyond challenging, I'd say.

@borovan
Copy link
Author

borovan commented May 7, 2022

https://forum.dfinity.org/t/reversing-an-array/12724/18?u=borovan

Yeah maybe icme has a point. The methods in Iter are probably fine but you just have to use Int.abs() on the result within an array.

Just wanted you see it from the developers perspective.

@ggreif ggreif force-pushed the master branch 2 times, most recently from d52aecd to 08507fc Compare October 21, 2022 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants