Skip to content

Conversation

marcinczenko
Copy link
Contributor

@marcinczenko marcinczenko commented Jun 3, 2025

Continuation of #1179.

First part:

  • get rid of the old AsyncIter in favor of SafeAsyncIter
  • rename SafeAsyncIter -> AsyncIter after replacement

After that there will be separate PR that aims at replacing all remaining {.async.} with something closer to {.async: (raises: [CancelledError]).}. I decided to split the PR as otherwise we may miss some important things related to erasure coding that should be reviewed.

@marcinczenko marcinczenko marked this pull request as draft June 3, 2025 00:58
@dryajov
Copy link
Contributor

dryajov commented Jun 5, 2025

Actually, thinking about this now, maybe we should leave both SafeAsyncIter and AsyncIter, it doesn't seem like they can be unified easily and it does seem like AsyncIter could be useful on it's own.

@marcinczenko marcinczenko force-pushed the checked-exceptions-cont branch from 8cc1b35 to 8178f3d Compare June 17, 2025 02:13
@marcinczenko
Copy link
Contributor Author

marcinczenko commented Jun 17, 2025

@dryajov

Actually, thinking about this now, maybe we should leave both SafeAsyncIter and AsyncIter, it doesn't seem like they can be unified easily and it does seem like AsyncIter could be useful on it's own.

I am hesitating a bit, but perhaps we could keep the old AsyncIter. If we want to keep it, we probably should change the names. For now I have renamed the SafeAsyncIter to AsyncIter (so that it is the default). Now if we decide to resurrect the old AsyncIter we should reconsider the naming: maybe let's call it UnsafeAsyncIter or AsyncIterWithExceptions or ThrowingAsyncIter or RaisingAsyncIter? What do you think? In any case I feel that the name AsyncIter should be used by the default one. I am also fine with keeping the old naming: SafeAsyncIter for the new one that is used, and AsyncIter for the legacy one which is not currently used anymore.

Another thing - If we decide to keep the old AsynIter - is that we should probably refactor it slightly, as some of its types are borrowed from regular sync Iter so it was even missing proper .async. annotations. It is just a bit legacy code.

Or even better, and something you also mentioned I think - perhaps, it is best to move them to a separate package in a separate repo - both of them in the same repo I suppose: something like "nim-async-iterators".

Please take a look also at codex-storage/codex-pm#238 for some naming proposals.

Let me know what do you think.

@marcinczenko marcinczenko marked this pull request as ready for review June 17, 2025 08:29
@marcinczenko marcinczenko requested review from cnanakos and dryajov June 17, 2025 08:29
@marcinczenko marcinczenko force-pushed the checked-exceptions-cont branch 2 times, most recently from b5c3695 to dd7e4a7 Compare June 24, 2025 13:28
@marcinczenko
Copy link
Contributor Author

I have aligned the tree iterators, improved separation between them (so that e.g. AsyncIter does not reuse Iter's types that are not meant for asynchronous calls), and I made sure that the annotations are more align with "modern" Nim. Thus in the end we have three iterators: Iter, AyncIter (one that can throw exceptions), and AsyncResultIter (one that uses Result to communicate errors and failures). From those three, AsyncIter is currently not in use. They are all ready to be extracted to a separate Nim package.

@marcinczenko marcinczenko force-pushed the checked-exceptions-cont branch 2 times, most recently from 8b6a7e2 to 22954d1 Compare June 24, 2025 23:50
@marcinczenko marcinczenko force-pushed the checked-exceptions-cont branch 2 times, most recently from d3780d2 to 5c379b4 Compare June 25, 2025 11:33
Copy link
Contributor

@cnanakos cnanakos left a comment

Choose a reason for hiding this comment

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

Great work!

@marcinczenko marcinczenko force-pushed the checked-exceptions-cont branch from 227e88c to 002e717 Compare June 26, 2025 09:25
@marcinczenko marcinczenko force-pushed the checked-exceptions-cont branch from 002e717 to f6a0e9d Compare July 3, 2025 10:47
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.

3 participants