-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
Introduce Oxpecker.ModelValidation #33
Conversation
match dict.TryGetValue(memberName) with | ||
| true, value -> value.Add(error.ErrorMessage) | ||
| false, _ -> | ||
let arrayList = ResizeArray(1) | ||
arrayList.Add(error.ErrorMessage) | ||
dict[memberName] <- arrayList |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as an option
match dict.TryGetValue(memberName) with | |
| true, value -> value.Add(error.ErrorMessage) | |
| false, _ -> | |
let arrayList = ResizeArray(1) | |
arrayList.Add(error.ErrorMessage) | |
dict[memberName] <- arrayList | |
if not (dict.Contains(memberName)) then dict[memberName] <- ResizeArray(1) | |
dict[memberName].Add(error.ErrorMessage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will cause more dict access
```fsharp | ||
[<RequireQualifiedAccess>] | ||
type ModelState<'T> = | ||
| Empty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe NotValidated
to prevent any collision.
However, if you require qualified access this is fine too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, RequireQualifiedAccess
resolves any collisions
src/Oxpecker/ModelValidation.fs
Outdated
member this.Value(f: 'T -> string | null) = | ||
match this with | ||
| Empty -> null | ||
| Valid model -> f model | ||
| Invalid(model, _) -> f model | ||
member this.BoolValue(f: 'T -> bool) = | ||
match this with | ||
| Empty -> false | ||
| Valid model -> f model | ||
| Invalid(model, _) -> f model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
member this.Value(f: 'T -> string | null) = | |
match this with | |
| Empty -> null | |
| Valid model -> f model | |
| Invalid(model, _) -> f model | |
member this.BoolValue(f: 'T -> bool) = | |
match this with | |
| Empty -> false | |
| Valid model -> f model | |
| Invalid(model, _) -> f model | |
private member this.Apply(f: 'T -> 'U) = | |
match this with | |
| Empty -> Unchecked.defaultof<'U> | |
| Valid model -> f model | |
| Invalid(model, _) -> f model | |
member this.Value(f: 'T -> string | null) = this.Apply f | |
member this.BoolValue(f: 'T -> bool) = this.Apply f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I get the duplication point, I like the readability of the current implementation more (and it doesn't use Unchecked
module)
type ValidationErrors(errors: ResizeArray<ValidationResult>) = | ||
let errorDict = | ||
lazy | ||
(let dict = Dictionary<string, ResizeArray<string|null>>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to do this nullable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/Oxpecker/ModelValidation.fs
Outdated
| Empty | ||
| Valid of 'T | ||
| Invalid of InvalidModel<'T> | ||
member this.Value(f: 'T -> string|null) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be better/more idiomatic as an Option<string>
? Curious why the nullability here. I don't write F# in my day job, so I might be missing some nuance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, the nuance is that it should be mostly used to set value
attribute of the input and that has type string | null
for performance and DX reasons. So I tried to match types here to avoid unnecessary conversions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that makes a lot of sense! Thanks for the explanation
Validation model based on standard ASP.NET Core validation mechanism based on
System.ComponentModel.DataAnnotations
attributes.