Fixes #1937 string iterator suffix #1938
Conversation
Note, when I tested this I noticed that although the FOR loop now works, the :RESET suffix of the iterator you get from String:ITERATOR doesn't work. :RESET throws a System.NotSupportedException. When I looked in to why, I tried comparing it to some of the other iterators for Queue, List, and so on. **Many of them give the same error**, but our docs mention being able to do a RESET of an Iterator. I didn't check which types don't implement it but several of ours don't. Our documentation is wrong. I made a quick edit so at least the docs aren't lying, but it should be addressed better in a future issue. The iterators we are using have no RESET on them. The suffix exists, but it points to a method under the hood that's not there. (Basically, our iterators implement C#'s IEnumerator, but apparently IEnumerator has a method called Reset() that you are guaranteed the compiler will allow you try calling even though IEnumerator isn't required to actually be implemented in classes that claim to implement IEnumerator. This seems to hint at the existence of something I've never heard of before in C# - an interface method you aren't required to implement because there's some kind of default base behavior when you don't. (Isn't that by defintion an Abstract Class instead of an Interface then if it has some implemented and some not implemented methods?))
I found out that we had to remove the :RESET suffix entirely
because the way the new collections structures we have
no longer actually support rewinding the enumerators:
When we went with the newer collections style, there
was a revamp to go through all of them and change how
they returned ITERATORS. Instead of returning a hand-made
IEnumerator, the technique was changed to the one where you
let C# magically create an IEnumerator<fooType> out of
a single method where all it does a yield return <fooType>.
Apparently this technique causes it to build an IEnumerator
in which the .Reset() method has been defined as just
public void Reset()
{
throw new System.NotSupportedException().
}
So this part of our docs have been lying ever since we made that
change. The RESET was never implemented an never worked ever
since then.
Dunbaratu
added
the
bug
|
Trying to understand how yield return makes an IEnumerator was a pain to figure out. It's not that the concept of yield returns itself is hard (return a value but the next time you're called pick up here where you left off instead of at the start of the method). That all made sense. What was hard to find was the fact that the yield keyword also wraps the return value inside an Enumerator and returns the Enumerator instead. The apparent mismatch between the return type in the method body and the return type in the method declaration is baffling if you don't know the keyword has that effect. (It's weird because that means one class in the .net library has been given its own special keyword in the C# language. That doesn't feel right because it violates the wall of separation between compiler and library.) |
| @@ -127,6 +169,9 @@ Structure | |||
| * - :meth:`INSERT(index, string)` | |||
| - :struct:`String` | |||
| - Returns a new string with the given string inserted at the given index into this string | |||
| * - :attr:`ITERATOR` | |||
There was a problem hiding this comment.
In all of our other structures, we don't duplicate suffixes from inherited structure types. But this table doesn't direct the user to go look for Enumerator. I think I will add that reference and delete the ITERATOR documentation in this file if that's OK with you. It also fixes an error since Sphinx is complaining that it's defined twice.
There was a problem hiding this comment.
Does string inherit from Enumerator?
There was a problem hiding this comment.
Oh hey, you're right. It is literally just an iterator suffix, not the rest of the enumerator stuff... I take it all back. I'll just remove the enumerator portion of that attribute definition below instead. Ignore my craziness.
|
|
||
| Strings can be treated a little bit like iterable lists | ||
| of characters. This allows them to be used in FOR loops | ||
| as in the example below: |
There was a problem hiding this comment.
Need to add a : for the code example to work. I'll handle it in my review edits.
| } | ||
|
|
||
| The reason you can use Strings with the FOR loop like this is | ||
| because you can obatain an :struct:`Iterator` of a string with the | ||
| :attr:`ITERATOR` suffix mentioned below. (Any type that |
There was a problem hiding this comment.
Because I'll be pointing to Enumerator below, I'll edit this to match.
|
I have committed the changes I mentioned above to my local branch. When you get a chance to look at my comments, if you agree with them I can just push them upstream. |
Fixes #1937 by adding the ITERATOR suffix to StringValue.
In the process of testing this I found that the :RESET method of that iterator was throwing errors. In trying to fix that and comparing it to our other collection iterators that I assumed worked, I found out that their :RESETs didn't work either and have had the same problem for a long time now.
See the commit log messages for further explanation - but basically the problem with RESET cannot be fixed. We're using that
yield returnmagic for making iterators, and the kinds of iterators that this creates don't have Reset().(I considered adding the "breaking" label because I'm removing a previously existing suffix, but I didn't because technically this suffix was already broken before and threw errors when used - this will just change which kind of error message people will see.)