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

Make copy-and-update syntax consistent with records #17

Closed
wants to merge 1 commit into from

Conversation

knocte
Copy link
Contributor

@knocte knocte commented Feb 3, 2021

Long records are aligned this way:

let rainbow =
    {
        Boss = "Jeffrey"
        Lackeys = [ "Zippy" ; "George" ; "Bungle" ]
    }

So copy-and-update syntax should be similar:

let rainbow2 =
    {
        rainbow with
            Boss = "Jeffrey"
            Lackeys = [ "Zippy" ; "George" ; "Bungle" ]
    }

Long records are aligned this way:
let rainbow =
    {
        Boss = "Jeffrey"
        Lackeys = [ "Zippy" ; "George" ; "Bungle" ]
    }

So copy-and-update syntax should be similar:
let rainbow2 =
    {
        rainbow with
            Boss = "Jeffrey"
            Lackeys = [ "Zippy" ; "George" ; "Bungle" ]
     }
@Smaug123
Copy link
Contributor

Smaug123 commented Feb 3, 2021

Internally we have a strong convention of:

let rainbow2 =
    { rainbow with
        Boss = "Jeffrey"
        Lackeys = [ "Zippy" ; "George" ; "Bungle" ]
    }

@nojaf
Copy link
Contributor

nojaf commented Feb 3, 2021

In regards to fsprojects/fantomas#1396, we are more wondering if anything changes if the original value is a multiline expression.

Is it

    { GeeTower.Backend.Configuration.GetTestingConfig(
        lndAddress.ToString()
      ) with
        BitcoinRpcUser = "btc"
    }

or

    {
        GeeTower.Backend.Configuration.GetTestingConfig(
            lndAddress.ToString()
        ) with
            BitcoinRpcUser = "btc"
    }

I had no intention to challenge the current guide.

@Smaug123
Copy link
Contributor

Smaug123 commented Feb 3, 2021

Oh, I see. Both options look really horrible to me :P I'll ping this over to Toby for discussion.

@TobyShaw
Copy link

TobyShaw commented Feb 3, 2021

I have never seen the record expression in a record update expression be multi line. It's always assigned to an intermediate variable first.

I have no strong opinions on which is chosen to resolve this, since I wouldn't approve any code which contains either expression.

@nojaf
Copy link
Contributor

nojaf commented Feb 4, 2021

To me, it would seem more consistent to have it like

    { GeeTower.Backend.Configuration.GetTestingConfig(
        lndAddress.ToString()
      ) with
        BitcoinRpcUser = "btc"
    }

but indeed, I'd also go for an intermediate variable first.

@knocte knocte closed this Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants