avh4 / elm-format

elm-format formats Elm source code according to a standard set of rules based on the official Elm Style Guide
BSD 3-Clause "New" or "Revised" License
1.31k stars 148 forks source link

Automatic multiline `let` bindings #89

Closed rtfeldman closed 8 years ago

rtfeldman commented 8 years ago

Currently elm-format will not alter this:

let
    foo = bar baz

I've been following @evancz's formatting suggestion of always having newlines after = in let bindings, which would suggest elm-format should automatically change the above to:

let
    foo =
        bar baz

I'd be in favor of having it always do the latter. (Currently I just always make this change by hand when porting old code.)

Thoughts?

avh4 commented 8 years ago

I left this in because in the beginning I had seen a lot of code where the let bindings were very short, so splitting the lines AND inserting blank lines between every one scared me a bit.

Do you have any typical examples of using lets that you can link to here?

evancz commented 8 years ago

I'd use something like this as an example. In the original version of this module, everything was on one line. I eventually had to go through for some reason or another, and I could not figure out what was going on at all. Everything had one letter names, everything was all crushed into big solid blocks of "one-liner" definitions in a let. The file has still not recovered entirely from this.

As an aside, this may also encourage people to give helpful names instead of things like k. That looks really dumb when there is a newline, and the cool thing is that it is really dumb! Hopefully that'd encourage users to describe what's going on there. (Are people worried they will run out of characters if they use too many at once?)

avh4 commented 8 years ago

Here's the "worst"-case of single-line let bindings from some of my own code: https://github.com/avh4/t1d-treatment/blob/master/Glucose/Simulation.elm#L43-L63

This may not be typical of application code in Elm, but would be more common with mathematical code or simulations. Would that code be better with all the bindings split out? I've never really considered that before.

rtfeldman commented 8 years ago

That looks really dumb when there is a newline, and the cool thing is that it is really dumb!

I LOL'd :smile:

Here's what that transformation would look like:

Before

step :
    Parameters
    -> TimeStep
    -> (IobUnit, CobUnit, BloodGlucoseReading)
    -> (IobUnit, CobUnit, BloodGlucoseReading)
step params event (iob0,cob0,bg0) =
    let
        bolus = event.bolus
        food = event.food.carbs
        basal = event.basal

        kCarb = params.correctionFactor/params.carbRatio
        kCorr = params.correctionFactor
        qBasal = params.requiredBasal
        iob = iob0 + bolus + basal/60
        cob = cob0 + food
        di = (1-iobDecay)*iob
        dc = (1-0.97)*cob
        bg = bg0 - (di-qBasal/60)*kCorr + dc*kCarb
    in
        (iob-di, cob-dc, bg)

After

step :
    Parameters
    -> TimeStep
    -> (IobUnit, CobUnit, BloodGlucoseReading)
    -> (IobUnit, CobUnit, BloodGlucoseReading)
step params event (iob0, cob0, bg0) =
    let
        bolus =
            event.bolus

        food =
            event.food.carbs

        basal =
            event.basal

        kCarb =
            params.correctionFactor / params.carbRatio

        kCorr =
            params.correctionFactor

        qBasal =
            params.requiredBasal

        iob =
            iob0 + bolus + basal / 60

        cob =
            cob0 + food

        di =
            (1 - iobDecay) * iob

        dc =
            (1 - 0.97) * cob

        bg =
            bg0 - (di - qBasal / 60) * kCorr + dc * kCarb
    in
        (iob - di, cob - dc, bg)
avh4 commented 8 years ago

I'm still not convinced this is the best thing, but I've implemented it in #103 so it will be included in the next alpha release to elm-dev.

spookylukey commented 7 years ago

This change seems to be against the style guide. I've struggled to find an official link to the Elm Style Guide, but it seems to be this - https://gist.github.com/laszlopandy/c3bf56b6f87f71303c9f - which says the following:

 calc box =
    let first = box.position.x
        second = box.position.y
    in
        first * second

Also, IMO the result of this is much uglier - for example with the code posted above, it has become far less readable. The natural symmetries between adjacent lines are lost due to splitting the code into multiple lines, making the code much harder to read.

I'd also argue it is a very unnatural style. Compare to the style used in technical papers. Some snippets from a paper I happen to have on my laptop, one prose-ish, one maths-ish:

in1p—the insertion of one penny; in2p—the insertion of a two penny coin; small—the extraction of a small biscuit or cookie; large—the extraction of a large biscuit or cookie; out1p—the extraction of one penny in change.

The alphabet of process P is denoted αP , e.g., αVMS = {coin, choc} αVMC = {in1p, in2p, small, large, out1p}

These are basically similar to the Elm Style Guide above. But if formatted according to the changeset above, they would be like:

in1p —
    the insertion of one penny;

in2p —
    the insertion of a two penny coin;

small —
    the extraction of a small biscuit or cookie;

large —
    the extraction of a large biscuit or cookie;

out1p —
    the extraction of one penny in change.

The alphabet of process P is denoted αP , e.g.,
    αVMS =
        {coin, choc}

    αVMC =
        {in1p, in2p, small, large, out1p}

This would be really unnatural and much harder to read. The lack of readability comes from a number of aspects:

Personally I think this also applies to top level definitions as well as let bindings, but at least there you might have type signatures that breaks things up anyway. For me, the changes to let bindings were the last straw - I could have lived with the other decisions, but with this, I had to decide not to use elm-format for my personal projects - the number of lines in one module increased by 67%, and the result was much uglier and harder to read IMO.

swelet commented 5 years ago

I agree with @spookylukey

Why should a simple

let hey = someQuickButUnavoidableThing bar in

become

let
    hey =
        someQuickButUnavoidableThing bar
in