lfr / FSharp.Domain.Validation

Designing with types requires a lot of code - this library fixes that
MIT License
143 stars 7 forks source link

New API #7

Closed lfr closed 3 years ago

lfr commented 3 years ago

Given that the type Block is about to be appropriated for immutable arrays, I'm contemplating a new API for this library. For now the best I can come up with is:

Block.validate // old
Block.value    // old

Box.validate   // new
Box.value      // new

I'm leaving this issue open here if anyone has suggestions.

In case you're wondering, it can't just be validate, it's part of the trickery that allows one to only specify one generic parameter when in fact two are needed (i.e. validate<Text> instead of validate<Text, TextError> or similar) which I'm sure everyone agrees is too convenient to let go.

TheAngryByrd commented 3 years ago

The only thing I'd say is let's try not to shadow/overlap functions in FsToolkit.ErrorHandling.Validation.

lfr commented 3 years ago

The only thing I'd say is let's try not to shadow/overlap functions in FsToolkit.ErrorHandling.Validation.

Thanks for pointing this out Jimmy, I'll keep your API in mind to avoid conflicts, also I have no intention of going monadic style with this lib so even if they have to coexist with the same module name (which is not at all a given at this point), they're clearly heading in different directions.

TheAngryByrd commented 3 years ago

It mean if we do overlap, might be worth for both of us to document it so users don't get confused :)

lfr commented 3 years ago

Incidentally, since this lib essentially generates results, it's very likely to be used with yours, so much so that I couldn't help myself from adding it to the live demo 🤷‍♂️

TheAngryByrd commented 3 years ago

Incidentally, since this lib essentially generates results, it's very likely to be used with yours, so much so that I couldn't help myself from adding it to the live demo 🤷‍♂️

Only nitpick I have is you probably want to use validation CE instead of result CE so it will combine multiple errors.

cmeeren commented 3 years ago

I don't use this library (yet), but one thing that struck me when reading the readme is that this library isn't about validation, it's about parsing. See Parse, don't validate. If the API is about to be changed anyway, I suggest changing "validate" to "parse".

Strictly speaking, the ITextBlock.Validate method actually performs validation, not parsing. But stuff like Block.validate should be called Block.parse.

cmeeren commented 3 years ago

Concerning Block: Since this is about constrained types, how about renaming Block to Constrained? If people use open type and a free-standing validate (or parse), it won't matter that much anyway.

Also, IConstrained seems intuitive:

type ConstrainedText = inherit IConstrained<string, TextError>
lfr commented 3 years ago

So I read that whole article, pretty interesting stuff, however I'm not too sure the general dev population understands parsing as a (better) form of validation yet, so for now since the main objective of the API is some familiarity for people who haven't been using this library before, I'm inclined to leave it as is. I may do a Twitter poll because I'm interested to know what other people think.

lfr commented 3 years ago

Only nitpick I have is you probably want to use validation CE instead of result CE so it will combine multiple errors.

I'm ashamed to say that I haven't had the chance to try it yet, but I'll do it asap and report back

@TheAngryByrd you're right, thanks for nitpicking, validation CE is exactly what I need, I've updated my example accordingly

lfr commented 3 years ago

Concerning Block: Since this is about constrained types, how about renaming Block to Constrained? If people use open type and a free-standing validate (or parse), it won't matter that much anyway.

Constrained was one of the original candidates, but it's a bit long, and even though open type exists today, it's still cumbersome to open type everywhere as you have to specifically indicate the interface whereas Block.validate is almost always inferred, you don't even need to remember what the generic primitive and error types are.

I'm now leaning towards Box.validate and Box.value, I've updated the original issue to reflect this.

cmeeren commented 3 years ago

I'm not too sure the general dev population understands parsing as a (better) form of validation yet

This may be true.

so for now since the main objective of the API is some familiarity for people who haven't been using this library before, I'm inclined to leave it as is.

This seems to build on a different assumption: not that people don't understand parsing as a "better form of validation", but that people are generally more familiar with the term "validate" than "parse". I'm not convinced this is the case. In any case, I don't think it's relevant; your API isn't that complicated, and whatever you call your methods/functions (Box.validate vs. Taco.parse), people will learn and remember it quickly.

Another benefit of parse is that it's' shorter.

The decision is yours, of course; this is just my two cents. I don't even use this library yet. 😜

lfr commented 3 years ago

Closed by mistake

lfr commented 3 years ago

The new API is now available by referencing the release candidate package 0.9.78-rc2.

@cmeeren the parse vs. validate is being tracked separately here.