fsharp / fslang-design

RFCs and docs related to the F# language design process, see https://github.com/fsharp/fslang-suggestions to submit ideas
518 stars 144 forks source link

Improve signature of defaultWith #698

Closed edgarfgp closed 2 years ago

edgarfgp commented 2 years ago

Click “Files changed” → “⋯” → “View file” for the rendered RFC.

Based on @gusty suggestion to improve the defaultWith signature : See : https://github.com/fsharp/fslang-suggestions/issues/1123#issuecomment-1179171067

edgarfgp commented 2 years ago

@cartermp would love to have a review please ?

cartermp commented 2 years ago

Thanks for the ping. I think this looks fine but would like @dsyme to give it a final look.

vzarytovskii commented 2 years ago

I don't remember if the current API was already released to nuget. If it did, this will technically be a breaking change (unit vs generic param).

edgarfgp commented 2 years ago

Hope we can fix this before released to nuget. If not we can add another function that adds this ? . I will more that happy to help either way :)

cartermp commented 2 years ago

@vzarytovskii I don't think it's in any shipped version, no. So there should be time? The PR that added the feature set was done in June 20th

dsyme commented 2 years ago

I'm ok with this as long as it hasn't been shipped to nuget

vzarytovskii commented 2 years ago

I'm ok with this as long as it hasn't been shipped to nuget

It hasn't been released as a nuget package but has been part of SDK/VS since June 20th(ish). Also, we have a beta package released about 2 weeks ago, as long as we fine with breaking betas, it's OK.

cc @KevinRansom

cartermp commented 2 years ago

By that you mean preview VS, right? Design changes like this are fine to make in previews I'd say (at least that's certainly the design for language and IDE features).

witoldsz commented 9 months ago

Is it just me, or did you change the type and meaning of the function, introducing a breaking change in F# programs?

My application stopped compiling when upgraded from F# 6

    [<CompiledName("DefaultWith")>]
    let defaultWith defThunk result =
        match result with
        | Error _ -> defThunk ()
        | Ok v -> v

to F# 7 with:

    [<CompiledName("DefaultWith")>]
    let defaultWith defThunk result =
        match result with
        | Error error -> defThunk error
        | Ok v -> v
vzarytovskii commented 9 months ago

Is it just me, or did you change the type and meaning of the function, introducing a breaking change in F# programs?

My application stopped compiling when upgraded from F# 6


    [<CompiledName("DefaultWith")>]

    let defaultWith defThunk result =

        match result with

        | Error _ -> defThunk ()

        | Ok v -> v

to F# 7 with:


    [<CompiledName("DefaultWith")>]

    let defaultWith defThunk result =

        match result with

        | Error error -> defThunk error

        | Ok v -> v

Are you sure you weren't using preview versions? It should only be a breaking change in previews.

gusty commented 9 months ago

@witoldsz either that (previews) or maybe you are using FsToolkit

witoldsz commented 9 months ago

I am pretty sure the version with Error _ -> defThunk () is in official dotnet/F# 6 version.

Let me just check:

$ dotnet --version
6.0.417
$ dpkg -l |grep dotnet-sdk
ii  dotnet-sdk-6.0                              6.0.417-1                                   amd64        Microsoft .NET SDK 6.0.417

The issue came up when my colleague tried to upgrade services to work with dotnet 8 and one stopped compiling because of the new meaning of the Result.defaultWith.