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 Array.take method #587

Merged
merged 4 commits into from
Dec 7, 2023
Merged

Add Array.take method #587

merged 4 commits into from
Dec 7, 2023

Conversation

ZenVoich
Copy link
Contributor

@ZenVoich ZenVoich commented Sep 5, 2023

Takes N elements from the start or end of the array

let array = [1, 2, 3, 4, 5];
assert Array.take(array, 2) == [1, 2];
assert Array.take(array, -2) == [4, 5];
assert Array.take(array, 10) == [1, 2, 3, 4, 5];
assert Array.take(array, -99) == [1, 2, 3, 4, 5];

@ghost ghost added the cla:agreed label Sep 5, 2023
@ZenVoich
Copy link
Contributor Author

ZenVoich commented Oct 3, 2023

Any feedback would be appreciated)

@ZenVoich
Copy link
Contributor Author

Anything?

Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

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

This looks good to me and thanks for the test. Sorry for the delay.

One possible objection is that the existing List.take has a less general signature and only takes a Nat, not an Int.

I'm ok with that but what do others think @rvanasa @ggreif @luc-blaeser @chenyan-dfinity?

Also, do we want a version for mutable arrays too?

@rvanasa
Copy link
Contributor

rvanasa commented Dec 6, 2023

The more general signature seems okay to me because it's still possible to pass a Nat without any additional conversions (example). The +/- convention is reasonably intuitive for developers familiar with Python or JS.

Also, do we want a version for mutable arrays too?

+1

@crusso
Copy link
Contributor

crusso commented Dec 7, 2023

Also, do we want a version for mutable arrays too?

+1

Looking at the module api, I see that most (but not all) operations only operate on immutable arrays. Instead of adding a mutable variant of just this operation ( takeVar or takeVarArray?), perhaps we should consider later adding a new module VarArray with mutable versions of all relevant operations (VarArray.take, VarArray.filter etc).

So I'd say let's just merge this immutable Array.take for now until we find a suitable name or home for the mutable version.

We already have some historically inconsistent naming that it would be nice to resolve, eg Blob.fromArrayMut (not Blob.fromVarArray).

Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution and sorry for the delay!

@crusso crusso merged commit be273ff into dfinity:master Dec 7, 2023
6 checks passed
@ZenVoich ZenVoich deleted the array-take branch December 7, 2023 11:53
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.

3 participants