Skip to content

Conversation

hoosan
Copy link
Contributor

@hoosan hoosan commented Feb 13, 2022

In this PR, I have added a method toIter to List.mo to convert List to Iter.
The first reason for this addition is that it is intuitive to have the conversion method defined in the class/module from which it is converted, in association with for example Rust's array.into_iter().
The second reason is that the current library requires the conversion from List to Iter using Iter.fromList(), so there are situations where an extra import Iter "mo:base/Iter"; is needed just for the conversion.
Thank you for your consideration.

@ggreif ggreif requested a review from crusso February 15, 2022 13:15
q-uint
q-uint previously approved these changes Mar 23, 2022
@matthewhammer
Copy link
Contributor

matthewhammer commented Mar 23, 2022

Thanks for this contribution @hoosan!

In the review above, I see that @ggreif raised the issue of naming this function. To be consistent with the guidelines for base we may consider vals here, as he pointed out.

FWIW, I think toIter is okay here. Though the guidelines do not seem to specify a distinction, in practice I see a difference between names used in modules that implement containers and classes that implement them. Looking at how we use this name so far, I think of vals as being a class method, not a module function.

Despite the guidelines saying otherwise, I also tend to think of toIter as being a reasonable name for the latter, as we have other type-conversion functions that use that naming convention (like toArray in module Blob, etc).

@crusso
Copy link
Contributor

crusso commented Mar 23, 2022

Yeah, I'm fine with toIter since this is a library function, not a member or special member of a class or type. But I don't feel that strongly about the name.

I'd actually suggest replacing the Iter.fromList implementation with this one (perhaps just defining public let fromList = List.toIter, since this one is more efficient. The existing fromList allocates an array via a buffer which seems much less direct and efficient than this. Do you think you could redefine Iter.fromList as described in this PR too?

(I guess the argument for calling it vals would be stronger if we supported l.vals() as syntactic sugar for List.vals(l) (a la C# extensions methods), but we don't do that yet and perhaps never will.)

Thanks for the contribution!

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.

If you don't mind, please also re-implement Iter.fromList directly using List.toIter.

@hoosan
Copy link
Contributor Author

hoosan commented Mar 30, 2022

Thank you @matthewhammer and @crusso for your kind reviews. I replaced the implementation of Iter.fromList to use List.toIter just as proposed by @crusso .

@hoosan hoosan requested a review from crusso March 30, 2022 22:45
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 @hoosan!

@crusso crusso enabled auto-merge (squash) April 15, 2022 09:05
@crusso crusso merged commit 8d46fd5 into dfinity:master Apr 15, 2022
@hoosan hoosan deleted the toiter branch April 15, 2022 09:52
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.

5 participants