-
-
Notifications
You must be signed in to change notification settings - Fork 48
Make Array iterator classes private to hide implementation from client #193
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
e6691a1
84535ad
42b09a7
5f5d79f
a52eb5f
73099ee
873c378
b0fd354
f492370
62a7b03
48fc8bd
255c487
869eb82
2bfd088
7bf7641
06018fc
e60bb5a
16ec9ef
5b91ce2
7fda29d
4363ab0
5dbe1ca
68b7695
c0da56d
b539018
44103dd
d61d656
18f8d49
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| - Feature Name: array_private_classes | ||
| - Start Date: 2022-01-08 | ||
| - RFC PR: (leave this empty) | ||
| - Pony Issue: (leave this empty) | ||
|
|
||
| # Summary | ||
|
|
||
| Using information hiding design principle, the builtin classes `ArrayKeys`, `ArrayValues` and `ArrayPairs` can be made private as they are only used as return types for `Array` functions `keys`, `values` and `pairs`. The return values for these functions is be changed to general `Iterator`. A new interface `Rewindable` is defined to allow for rewindable iterators. | ||
|
|
||
| # Motivation | ||
|
|
||
| This change brings: | ||
|
|
||
| - Applying the design principle of [hiding implementation details](https://en.wikipedia.org/wiki/Information_hiding) but offer a general and stable interface. Returning interfaces instead of concrete classes allows changing the implementation. | ||
| - `Array` functions `keys`, `values` and `pairs` definitions are made more general. `Array` iterators implementation details are not public. `_ArrayKeys`, `_ArrayValues` and `_ArrayPairs` are now [opaque data types](https://en.wikipedia.org/wiki/Opaque_data_type). | ||
| - Makes the standard library more simple by removing 3 specialised classes. | ||
| - The interface `Rewindable` can be combined to create rewindable iterators or other types that can be rewinded. | ||
|
|
||
| This change is compatible with the existing code base but for client code that is directly using the classes `ArrayKeys`, `ArrayValues` and `ArrayPairs`. A search on Github shows that the impact is very limited. | ||
|
|
||
| # Detailed design | ||
|
|
||
| `Array` functions `keys`, `values` and `pairs` are changed to return `Iterator` and the classes that implement these iterators are make private. Here are the full implementation of these functions. | ||
|
|
||
| ```pony | ||
| fun keys(): Iterator[USize]^ => | ||
| """ | ||
| Return an iterator over the indices in the array. | ||
| """ | ||
| _ArrayKeys[A, this->Array[A]](this) | ||
|
|
||
| fun values(): (Iterator[this->A] & Rewindable[this->A]) => | ||
|
pmetras marked this conversation as resolved.
Outdated
|
||
| """ | ||
| Return an iterator over the values in the array. | ||
| """ | ||
| _ArrayValues[A, this->Array[A]](this) | ||
|
|
||
| fun pairs(): Iterator[(USize, this->A)]^ => | ||
| """ | ||
| Return an iterator over the (index, value) pairs in the array. | ||
| """ | ||
| _ArrayPairs[A, this->Array[A]](this) | ||
| ``` | ||
|
|
||
| As the function `values` uses an iterator with a `rewind` function that is not part of the `Iterator` interface, a new interface `Rewindable` is added to enbale creation of rewindable iterators. | ||
|
|
||
| ```pony | ||
| interface Rewindable[A] | ||
| """ | ||
| An `Iterator` is rewindable when it can be rewinded, that is start again from | ||
| first value. | ||
| """ | ||
| fun ref rewind(): Iterator[A]^ | ||
| ``` | ||
|
|
||
| The code of the standard library is adapted to remove use of these now private classes, mainly in tests. Here are the files that must be changed: | ||
|
|
||
| * `iter.pony` in function `cycle` | ||
| * `heap.pony` in function `values` | ||
| * `_test.pony` in class `_TestArrayValuesRewind` | ||
| * `util.cc` to change the name of the class to `_ArrayValues` | ||
|
|
||
| # How We Teach This | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nothing in this section seems to be about teaching people anything going forward about the pattern nor explaining to people impacted by the breaking change what they need to be aware of. This section should be devoted to what if anything should be done with Pony Patterns, the Pony tutorial, standard library documentation, and release notes to adapt to the new world this RFC would bring about.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @pmetras - "How We Teach This" could include a post on the "Pony Patterns" site that demonstrates how to use a public interface to hide the implementation of a private class, rather than making that class public. Like you, I see this as a desirable design pattern, so we could have a "Pony Patterns" example showing how we did this in the Pony standard library with |
||
|
|
||
| This change keeps the code compatible in most cases. When client classes are defining objects of these types, the reason is usually to get access to the function `rewind` that was not defined in `Iterator`. By adding the interface `Rewindable`, client code can easily be adapted, replacing `ArrayValues[A]` by `(Iterator[A] & Rewindable[A])`. A [search on Github Pony code](https://github.com/search?q=%22ArrayValues%22+language%3APony&type=code) finds 24 files using the class `ArrayValues`, of which 6 are copies of `array.pony` file. | ||
|
|
||
| For instance, in [xml2xpath.pony](https://github.com/redvers/pony-libxml2/blob/bbca5d98d48854bfec2c6ee110220873ecc4df34/pony-libxml2/xml2xpath.pony#L41), the code can be changed from | ||
|
|
||
| ```pony | ||
| fun values(): ArrayValues[Xml2node, this->Array[Xml2node]]^ ? => | ||
| if (allocated) then | ||
| ArrayValues[Xml2node, this->Array[Xml2node]](nodearray) | ||
| else | ||
| error | ||
| end | ||
| ``` | ||
|
|
||
| to | ||
|
|
||
| ```pony | ||
| fun values(): (Iterator[Xml2node] & Rewindable[Xml2node]) ? => | ||
| if (allocated) then | ||
| nodearray.values() | ||
| else | ||
| error | ||
| end | ||
| ``` | ||
|
|
||
| This change in `array.pony` will break such code but it can be easily adapted to use the new API. And it will make the standard library easier when reducing the number of builin types exposed. | ||
|
|
||
| # How We Test This | ||
|
|
||
| Pony tests must continue to pass. | ||
|
pmetras marked this conversation as resolved.
Outdated
|
||
|
|
||
| # Drawbacks | ||
|
|
||
| As said, some client's code must me adapted when using these classes. As these classes are just concreted implementations of `Iterator` for use by `Array`, their use in client code is limited and the code can very easily be changed. | ||
|
|
||
| # Alternatives | ||
|
|
||
| Stay as is with these 3 builtin classes. Continue the [discussion on Zulip](https://ponylang.zulipchat.com/#narrow/stream/189959-RFCs/topic/Make.20Array.20iterators.20private). | ||
|
|
||
| # Unresolved questions | ||
|
|
||
| None | ||
Uh oh!
There was an error while loading. Please reload this page.