Is there anything missing from IReader(Async) and IWriter(Async)? #11
CesilUtils also exposes helpers for most of those methods, so failings in it may also imply failings in the interfaces.
This is an Open Question, which means:
The text was updated successfully, but these errors were encountered:
A few questions (about the Async interfaces, I didn't look much into the implementations):
Isn't this missing the cancellationtoken and if yes, you probably need that special attribute
Why the class constraint? to make the IL gen easier?
ref in async is a bit strange, do you need the ref for your 'If need be, row will be initialized before'?
This is kind of a tricky bit of async enumerators, but I believe not taking a CancellationToken is correct here.
We're returning an IAsyncEnumerable, CancellationToken is used by a IAsyncEnumerator. You use
Put another way, EnumerateAsync() doesn't actually do any work or start any tasks - work is deferred until the first call to
If you're not going to explicit invoke GetAsyncEnumerator(), .NET has
Ah, that is to prevent a footgun - if
I suppose by only passing the collection around internally by ref we can avoid breaking value type collections, though it'd still be up to the client to make sure they always look at the returned, probably modified, copy.
It really is a question of if supporting value type collections is worth extra complexity (there may be a tiny perf hit due to the extra indirection, but not enough to matter IMO). I'm kind of leaning towards "yes, remove the
You're technically correct that an
The way I implement
You didn't ask, but it sort of naturally follows: "why doesn't
That's a good idea, any of the "write multiple" methods really should return a written row count.
That is also a good point, fortunately I have some tests around parameter names... so cancellationToken can be easily enforced everywhere.
Thank you for your detailed answer and I think it dawned on me, the consumer never ever implements these interfaces, he only consumes them, right?
After looking at your implementation, you return a custom type for it
I think it would be useful to remove the constraint, the only collection type I commonly use which is a struct is
Yes, my point is mood if only you implement the interface, because while the sync completion pattern for ValueTask via
Yeah I saw that and just assumed it's about symmetry with your other
And that reminds me of something I forgot about in IAsyncWriter, I think your
ValueTask WriteCommentAsync(string comment, CancellationToken cancel = default) => WriteCommentAsync(comment.AsMemory(), cancel); ValueTask WriteCommentAsync(ReadOnlyMemory<char> comment, CancellationToken cancel = default);
Apropos, I think you can safely increase the TFM of your cesil.csproj to 3.1, as 3.0 is out of maintenance
That is correct, the interfaces are only meant to be implemented by Cesil code.
Immutable collections are actually even trickier, to add to them you have to capture the return value but since there's no common interface for the immutable collections it's not really possible to do that generically (that I have found anyway, would love to be proven wrong there). Instead you have to pass the builders (like
Cesil does have code to detect that a known immutable collection has been passed, and throw with advice to pass a builder instead.
Still planning to explore removing the
Yeah, they're threading a narrow use case: you really care about allocations, but don't want to go so far as to explicit implement object pooling as part of a custom
That's a good idea, Cesil should support
If we're just talking about consuming:
Ah yes, I forgot
I personally think that's a nice solution for that problem.
I spent some time exploring removing the
None of the above applies to
New comment overloads have been implemented in #16 .
I think that covers all feedback to date on this issue, with the exception of moving from .NET Core 3.0 to 3.1. That is planned, I'm just going to wait until after the (already drafted) blog posts that explicitly refer to 3.0 are all published.
I'm going to spend some time getting all PRs merged into vNext in the near future.