Skip to content
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

Fix fin enumerator #1247

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

StefanBertels
Copy link
Contributor

Fin<T> has wrong type in IEnumerable<>.

Side effect of the bug: XUnit will run into stack overflow if Assert.Equal() is used (see test).

Workaround: convert to Option<T> with e.g. myFin.ToOption()

@louthy
Copy link
Owner

louthy commented Sep 8, 2023

This probably needs a wider discussion. Because IEnumerable<> was only there to facilitate serialisation; if you look at Either<L, R>, you'll see that its IEnumerable<> bound-type is EitherData<L, R>. Again, this is because of the limitations of Json.NET - it was the only way I could reliably serialise without writing bespoke JsonConvertors (which I didn't want to do, because I didn't want to bring Json.NET into the Core).

I have however been investigating the idea of building bespoke LanguageExt.Serialisation.* libraries (where * would be Newtonsoft, System.Json, XmlDocument, Protobuf, etc. Which means there'd be no need for IEnumerable implementations on Option, Either, Fin, etc. Backwards compatibility could be maintained through some bespoke SelectMany operators, or just an expectation that conversion should be explicit, using AsEnumerable().

So, at the moment, your fix does fix an issue (honestly, I'm not sure what I was thinking when I wrote that!) - but it also makes it inconsistent with Either - probably, it should be IEnumerable<EitherData<Error, A>>. But, that whole line of reasoning will go away in about a month when I start building the new serialisation libraries!

Maybe just use Fin<A>.ToArray() temporarily, whilst I have a think about the best course of action.

For all: Please let me know if you have a strong opinion on this.

@StefanBertels
Copy link
Contributor Author

This all makes sense to me. I found it very practical to use IEnumerable<T> resp. SelectMany to mix up different container types but I see that this isn't clean and Either isn't compatible to this anyway. Being explicit is a good thing here because then we can really see that we "leave" the monadic type. But it should remain really easy to use Option etc. just like a container, maybe similiar to being able to enumerate key-value pairs in e.g. Map.

I agree that IEnumerable<EitherData<Error, A>> would be a good fit. This would allow really mixing Fin<A> with Either<Error,A>. Should they be really the same?

Using list pattern matching might improve user code so maybe get them together? Maybe .AsEnumerable() and .Case can both return the same type/interface that both implements IEnumerable<> and supports list matching (if the original type should not support list matching directly), maybe slicing. Seq?

Regarding serialization: Yes. Especially good System.Text.Json support would be great. I dropped Json.NET some time ago, System.Text.Json has a much cleaner design. I suggest to allow some configuration via attributes for better compatibility. Another discussion maybe...

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.

None yet

2 participants