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

Clarify constructors #11

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Clarify constructors #11

wants to merge 3 commits into from

Conversation

Smaug123
Copy link
Contributor

Sadly our "preferred" formatting is a syntax error due to offside indentation. That would be:

let thing = new Foobar(
    thing1,
    thing2
)

I've bitten the bullet and introduced another newline.

Aside: we are rather unclear on whether we want a space after the word Foobar here. I think sometimes it's a syntax error to insert a space.

@nojaf
Copy link
Contributor

nojaf commented Oct 30, 2020

At first glance, the other newline would be more consistent with anything I've seen so far.
To clarify I don't think there is any multiline construct that does still start right after the =.
For example:

let person = {
    Name = "barry"
}

So from an implementation point of view, I welcome the extra newline.

Does the example look the same when it is in an inner let binding?

let myFunction () =
    let thing =
        new FooBar (
             thing1,
             thing2
        )

    ()

@Smaug123
Copy link
Contributor Author

I think that looks right, except that I'm starting to think maybe it should be new FooBar( without the additional space before the bracket. I don't think we have enough of this kind of code for my opinions to be strong here; I'll ask the team.

@Smaug123
Copy link
Contributor Author

Looks like the rest of the team agrees that your suggestion is correct except for the space after the constructor (since I'm quite sure I remember there being times where new required there to be no space).

@nojaf
Copy link
Contributor

nojaf commented Nov 2, 2020

Is it by the way always a multiline construct or does that depend on the arguments of the constructor?

let thing = new Foobar(a, b)

let otherThing =
    new Foobar(
         thing1,
         thing2
    )

let m = new Meh(a) // what with only one argument?

@Smaug123
Copy link
Contributor Author

Smaug123 commented Nov 2, 2020

If the arguments fit on one line, we put them on one line:

let thing = new Foobar(a, b)
let otherThing =
    new Foobar(
        longname1,
        longname2
    )
let m = new Meh(a)

@nojaf
Copy link
Contributor

nojaf commented Nov 6, 2020

What if there is only one long argument?
Something like:

let myItem = new Foobar(someInitialStateThatHasQuietTheLongNameAndSeemsToGoOnAndOnAndOn)

Would it be

let myItem = 
    new Foobar(someInitialStateThatHasQuietTheLongNameAndSeemsToGoOnAndOnAndOn)

or

let myItem = 
    new Foobar(
        someInitialStateThatHasQuietTheLongNameAndSeemsToGoOnAndOnAndOn
    )

@Smaug123
Copy link
Contributor Author

Smaug123 commented Nov 6, 2020

I think we have no strong preference here; I'll pick arbitrarily.

let myItem = new Foobar(someInitialStateThatHasQuietTheLongNameAndSeemsToGoOnAndOnAndOn)

@knocte
Copy link
Contributor

knocte commented Nov 6, 2020

I'll pick arbitrarily.

But that choice makes it more likely to surpass the possible horizontal limit of the dev to avoid horizontal scrolling.

@Smaug123
Copy link
Contributor Author

Smaug123 commented Nov 6, 2020

This is predicated on the assumption that we can't fit it all on one line.

@nojaf
Copy link
Contributor

nojaf commented Nov 14, 2020

Slightly related does this rule also apply for member calls, static members?
Things like:

let myValue =
    Regex.Match(
        "my longer input string with some interesting content in it",
        "myRegexPattern"
    )

let untypedRes = 
    checker.ParseFile(
        fileName, 
        sourceText,
        parsingOptionsWithDefines
    )

@Smaug123
Copy link
Contributor Author

I'd say it applies for those as well, yes.

@nojaf
Copy link
Contributor

nojaf commented Nov 15, 2020

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.

3 participants