true-myth / true-myth-csharp

A library for safer optional programming in C♯.
https://true-myth.github.io/true-myth-csharp
MIT License
4 stars 1 forks source link

A re-envisioning of TrueMyth to be more idiomatic and native to C♯. #18

Closed aggieben closed 5 years ago

aggieben commented 5 years ago

Overview

The changes here are sweeping, but with the exception of .Apply (.ap in true-myth), this represents near-parity with the Javascript/TypeScript edition. Every element is documented (to varying degrees); most of it is a "translation" from the documentation on truemyth.js.org, with omissions and additions where the language differences demanded. There are a small handful of Javascript functions without equivalencies in C♯ where they should be implemented; these are annotated with "TODO?".

Major changes of note:

Mapping of functions

Unless otherwise indicated, all C♯ methods below are instance methods called directly on Result<T,E> or Maybe<T>. Also, this table will be in the documentation as well as here, as GitHub makes this harder to read than it could be.

Table moved to the user manual.

aggieben commented 5 years ago

@chriskrycho I'd say this is ready to review (when you get a chance - I don't plan to merge until next week sometime), even though there are a few gaps left to fill.

chriskrycho commented 5 years ago

In general I'm 100% behind this – and most of the decisions here look great.

The only ones I'd suggest revisiting are mapping the andThen functions to Select instead of SelectMany. C♯ developers are familiar with SelectMany via LINQ and the signature are the same, because SelectMany is bind/flatMap/chain/andThen.

Otherwise, I think the rest of the changes make sense.

As an aside: note that JS and TS can do overloading, although in a very different way, since JS functions can take and correctly handle dynamic numbers of parameters – and TS can actually correctly capture that (and indeed does so for the sake of automatic currying in True Myth).

aggieben commented 5 years ago

Yeah, those functions were a bit of a conundrum. I get that SelectMany is the representation of bind et al in LINQ, but it's also a bit of a special case because you're always dealing with sequences of things, and you don't ever see .SelectMany used like we do here. e.g., there's no Tuple<A,U> Tuple<T,U>.SelectMany<A,U>(Func<T,A> mapFn). Relatedly, SelectMany semantically refers to sequences, which we're mostly not dealing with here.

But having said that, I agree that T Result<T,U>.Select(T defaultValue) is not the same thing as Result<A,U> Result<T,U>.Select(Func<T,A> mapFn); I just don't know a better name for it that would still be idiomatic.

I'll dig around a bit more and see if I can come up with some alternatives.

chriskrycho commented 5 years ago

I entirely agree with everything you just said… but I also think it's equally applicable to Select. My original thought was: go with what C♯ users will find idiomatic, even if LINQ chose poorly when it came to naming these things, given that there was, in fact, prior art. 😆

aggieben commented 5 years ago

Well, ok, but at least .Select is semantically reasonable, even if it's a bit unconventional with respect to the terminology used in other stacks. SelectMany isn't even that, at least not in this context.

I'm staring to incline toward just using the usual monadic terms: .Map and .Bind, honestly. But if we're going to keep .Select around as an alias, what would you think about .SelectMap? Got any other ideas? I just don't know if I can live with .SelectMany. It makes me twitch.

chriskrycho commented 5 years ago

I think your best bet is either to just bite the bullet (maybe with an alias to make it less painful?) and do SelectMany, or to just use the normal names. My personal take is that .Select doesn't make sense – any sense! – for the scenario with Maybe or Result. Ultimately, though, I'll defer to you, @chrisriesgo, and any other C♯-ers who want to chime in here.

There's precedent for aliasing in True Myth: we supply aliases for compatibility and convenience for people migrating from other libraries, so you can use .andThen, .chain, or .flatMap. (We don't do .bind because that already has a meaning in JavaScript.)

aggieben commented 5 years ago

@chriskrycho @chrisriesgo I think I'm ready for you to have another look. I squashed all the azure pipelines stuff (at least I think I did), and now both Result and Maybe have .Map/.Bind with .Select/.SelectMany aliases.

The documentation is generated, but it's not hosted anywhere, and I think I'd prefer to work on that directly on the master branch because it will be easier to deal with github pages - so if there aren't any objections, I'd like to merge this and cut an rc4 release.