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

Built-in Result type not destructured #8

Closed
cmeeren opened this issue Dec 20, 2017 · 11 comments · Fixed by #10
Closed

Built-in Result type not destructured #8

cmeeren opened this issue Dec 20, 2017 · 11 comments · Fixed by #10
Labels
bug Something isn't working

Comments

@cmeeren
Copy link

cmeeren commented Dec 20, 2017

The FSharp.Core built-in Result type (defined here) is not destructured correctly:

Microsoft.FSharp.Core.FSharpResult`2[System.String,System.Object]

It seems it only runs ToString on it. I would expect this to be destructured the same way as other unions.

@nblumhardt nblumhardt added the bug Something isn't working label Dec 20, 2017
@nblumhardt
Copy link
Contributor

Thanks, @cmeeren 👍

If anyone has any ideas why this would be the case, a PR would be welcome.

@adamchester
Copy link
Contributor

Thanks @cmeeren, it seems to be due to this union being a ‘Struct‘ and we did not add support for that kind of union yet. I agree we should add support for this in the same way as other unions.

Thanks again 👍

@cmeeren
Copy link
Author

cmeeren commented Dec 20, 2017

Happy to be of help, hope it's a quick fix. ^_^

@adamchester
Copy link
Contributor

It should be a quick fix, we just need to find the APIs that we can use instead of this

            | t when FSharpType.IsUnion t ->
                let case, fields = FSharpValue.GetUnionFields(value, t)
                let properties = (case.GetFields(), fields) ||> Seq.map2 lep
                result <- StructureValue(properties, case.Name)
                true

@Zaid-Ajaj
Copy link

FYI, Suave.SerilogExtensions implements a proper destructuring mechanism for F# types (using Json -> LogEvent) see Destructure.fs

@adam-c-anderson
Copy link

Is this still really open? Just ran this in a test project:

Log.Logger <-
    LoggerConfiguration()
        .Destructure.FSharpTypes()
        .WriteTo.Console(Serilog.Formatting.Json.JsonFormatter())
        .CreateLogger()

type UnionData =
| Label of labelField:string
| Number of int

Log.Information("UnionData Label case {@label}", Label "Foo")
Log.Information("UnionData Number case {@number}", Number 42)
Log.Information("Result Ok case {@ok}", Ok "Bueno")
Log.Information("Result Error case {@error}", Error "Mal")

...and got this output

{"Timestamp":"2019-05-31T11:50:01.4639470-04:00","Level":"Information","MessageTemplate":"UnionData Label case {@label}","Properties":{"label":{"_typeTag":"Label","labelField":"Foo"}}}
{"Timestamp":"2019-05-31T11:50:01.4767652-04:00","Level":"Information","MessageTemplate":"UnionData Number case {@number}","Properties":{"number":{"_typeTag":"Number","Item":42}}}
{"Timestamp":"2019-05-31T11:50:01.4799127-04:00","Level":"Information","MessageTemplate":"Result Ok case {@ok}","Properties":{"ok":{"_typeTag":"Ok","ResultValue":"Bien"}}}
{"Timestamp":"2019-05-31T11:50:01.4807271-04:00","Level":"Information","MessageTemplate":"Result Error case {@error}","Properties":{"error":{"_typeTag":"Error","ErrorValue":"Mal"}}}

@baronfel
Copy link
Contributor

This same structure of struct-handling will hit struct options (valuesome/valuenone) as well.

@baronfel
Copy link
Contributor

I believe that all that needs to happen here is for #9 to be updated/finished and to upgrade to a more recent version of FSharp.Core (probably at least 4.6.2). Doing this will update the implementations of the FSharpType.Isxxxxx functions to return appropriate results for struct tuples, records, and unions. here's an example from the .net core FSI for 3.0 preview 7:

> let thing = ValueSome "ok";;
[<Struct>]
val thing : string voption = ValueSome "ok"

> let thingty = thing.GetType();;
val thingty : System.Type =
  Microsoft.FSharp.Core.FSharpValueOption`1[System.String]

> FSharp.Reflection.FSharpType.IsUnion thingty;;
val it : bool = true

@nblumhardt
Copy link
Contributor

Thanks for the notes, @baronfel . This library's in need of some maintenance, if you or anyone following along are keen to jump in and modernize things the help would be appreciated. Cheers!

@baronfel
Copy link
Contributor

I was planning on doing just that in a couple days/over the weekend. Are you a maintainer here (I'm not sure if the destructurama org is part of serilog proper)?

@nblumhardt
Copy link
Contributor

I'm a maintainer though I'm not actively maintaining :-) ... I haven't touched F# in nearly 10 years, unfortunately

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
6 participants