Avoid pointless line wrapping #180

New Issue

2018-09-13T18:06:30+02:00

mightybyte commented

2018-09-13 18:06:30 +02:00

(Migrated from github.com)

I just ran across a situation where this line:

    js "https://cdnjs.cloudflare.com/ajax/libs/ace/1.2.5/theme-solarized_dark.js"

...got wrapped to this:

    js
      "https://cdnjs.cloudflare.com/ajax/libs/ace/1.2.5/theme-solarized_dark.js"

This seems like the wrong thing to do. First of all, it's wrong because the wrapped version still exceeds the line length. Second, the wrapping only moves things left by a small amount. Even if the first reason wasn't the case, I think the second reason is still a valid reason for leaving a too-long line untouched.

This is a common issue in places where you have large string literals. Both of the above two criteria seem doable in terms of implementation feasibility, but additionally perhaps a third possible way of tackling this might be to just not try to wrap any line that contains a string literal longer than a certain length (say half the max line length or something similar).

Does anyone else have thoughts on this?

I just ran across a situation where this line: ``` js "https://cdnjs.cloudflare.com/ajax/libs/ace/1.2.5/theme-solarized_dark.js" ``` ...got wrapped to this: ``` js "https://cdnjs.cloudflare.com/ajax/libs/ace/1.2.5/theme-solarized_dark.js" ``` This seems like the wrong thing to do. First of all, it's wrong because the wrapped version still exceeds the line length. Second, the wrapping only moves things left by a small amount. Even if the first reason wasn't the case, I think the second reason is still a valid reason for leaving a too-long line untouched. This is a common issue in places where you have large string literals. Both of the above two criteria seem doable in terms of implementation feasibility, but additionally perhaps a third possible way of tackling this might be to just not try to wrap any line that contains a string literal longer than a certain length (say half the max line length or something similar). Does anyone else have thoughts on this?

ElvishJerricco commented

2018-09-13 18:39:35 +02:00

(Migrated from github.com)

  someReallyLongFunctionNameOrALongParenthesizedExpressionReturningAFunction "https://cdnjs.cloudflare.com/ajax/libs/ace/1.2.5/theme-solarized_dark.js"

should probably be formatted :P

  someReallyLongFunctionNameOrALongParenthesizedExpressionReturningAFunction
    "https://cdnjs.cloudflare.com/ajax/libs/ace/1.2.5/theme-solarized_dark.js"

So for me it seems hard to come up with good hueristics here. Maybe don't put something on a new line if it's only going to be moved over by like one space, and otherwise remain unchanged? I dunno, that breaks a lot of formattings that I prefer...

``` someReallyLongFunctionNameOrALongParenthesizedExpressionReturningAFunction "https://cdnjs.cloudflare.com/ajax/libs/ace/1.2.5/theme-solarized_dark.js" ``` should probably be formatted :P ``` someReallyLongFunctionNameOrALongParenthesizedExpressionReturningAFunction "https://cdnjs.cloudflare.com/ajax/libs/ace/1.2.5/theme-solarized_dark.js" ``` So for me it seems hard to come up with good hueristics here. Maybe don't put something on a new line if it's only going to be moved over by like one space, and otherwise remain unchanged? I dunno, that breaks a lot of formattings that I prefer...

lspitzner commented

2018-09-14 20:43:58 +02:00

(Migrated from github.com)

This seems like the wrong thing to do. First of all, it's wrong because the wrapped version still exceeds the line length.

Taken to the extreme, this view would mean that any function containing a literal longer than 78/80 characters (or any such identifier) could (and should) be layouted as a single, (however drastically) overflowing line. The argument "but do-notation should remain multiple-line" would only weaken this from "any function" down to "any statement".

And with that in mind, the next step is indeed to look for a less drastic heuristic.

I think it might boil down to something along the lines of "if the last child of any syntax construct is drastically long, layout the whole thing as if it was short". But I fear that rule alone would have several annoying false positives.

> This seems like the wrong thing to do. First of all, it's wrong because the wrapped version still exceeds the line length. Taken to the extreme, this view would mean that any function containing a literal longer than 78/80 characters (or any such identifier) could (and should) be layouted as a single, (however drastically) overflowing line. The argument "but do-notation should remain multiple-line" would only weaken this from "any function" down to "any statement". And with that in mind, the next step is indeed to look for a less drastic heuristic. I think it might boil down to something along the lines of "if the last child of any syntax construct is drastically long, layout the whole thing as if it was short". But I fear that rule alone would have several annoying false positives.

mightybyte commented

2018-09-17 01:19:08 +02:00

(Migrated from github.com)

@ElvishJerricco My heuristic of not wrapping if something is moved less than a certain amount to the left would handle your case.

@lspitzner One heuristic I have been thinking about is text "density"...in other words the percentage of non-whitespace characters visible on the screen. I don't know that this density metric should be universally maximized, but in the line wrapping situations I've thought brittany got wrong, it usually is reducing the density. All else being equal, the reformatting I encountered above significantly reduced the density. But for ElvishJerricco's suggested reformatting it would have increased the density. So this notion of density seems to be working decently thus far.

Consider another reformatting that brittany recently generated for me:

    elClass "div" "ui fluid action input" $ mdo
      name <- textInput $ def
          & textInputConfig_value .~ SetValue "" (Just $ "" <$ clicked)
          & textInputConfig_placeholder .~ pure "Enter key name"

...got reformatted to:

  elClass "div" "ui fluid action input" $ mdo
    name <-
      textInput
      $ def
      & textInputConfig_value
      .~ SetValue "" (Just $ "" <$ clicked)
      & textInputConfig_placeholder
      .~ pure "Enter key name"

This is substantially worse because none of the original lines exceeded the line length requirement. This particular example currently makes the use of brittany unacceptable for my project. Is there something we can do about this? Perhaps a config option that prevents brittany from reformatting lines shorter than the line length?

This is another example where brittany reduces the information density pretty significantly.

@ElvishJerricco My heuristic of not wrapping if something is moved less than a certain amount to the left would handle your case. @lspitzner One heuristic I have been thinking about is text "density"...in other words the percentage of non-whitespace characters visible on the screen. I don't know that this density metric should be universally maximized, but in the line wrapping situations I've thought brittany got wrong, it usually is reducing the density. All else being equal, the reformatting I encountered above significantly reduced the density. But for ElvishJerricco's suggested reformatting it would have increased the density. So this notion of density seems to be working decently thus far. Consider another reformatting that brittany recently generated for me: ```haskell elClass "div" "ui fluid action input" $ mdo name <- textInput $ def & textInputConfig_value .~ SetValue "" (Just $ "" <$ clicked) & textInputConfig_placeholder .~ pure "Enter key name" ``` ...got reformatted to: ```haskell elClass "div" "ui fluid action input" $ mdo name <- textInput $ def & textInputConfig_value .~ SetValue "" (Just $ "" <$ clicked) & textInputConfig_placeholder .~ pure "Enter key name" ``` This is substantially worse because none of the original lines exceeded the line length requirement. This particular example currently makes the use of `brittany` unacceptable for my project. Is there something we can do about this? Perhaps a config option that prevents brittany from reformatting lines shorter than the line length? This is another example where brittany reduces the information density pretty significantly.

mightybyte commented

2018-09-17 19:04:34 +02:00

(Migrated from github.com)

Here's another example where brittany's line wrapping is egregiously bad. This:

    onNewKey <- performEvent $ createKey signingKeys <$>
      -- Filter out keys with empty names
      ffilter (/= "") (conf ^. walletCfg_onRequestNewKey)

...got reformatted to:

    onNewKey <- performEvent $ createKey signingKeys <$>
      -- Filter out keys with empty names
	                                                       ffilter
     (/= "")
     (conf ^. walletCfg_onRequestNewKey)

Here's another example where brittany's line wrapping is egregiously bad. This: ```haskell onNewKey <- performEvent $ createKey signingKeys <$> -- Filter out keys with empty names ffilter (/= "") (conf ^. walletCfg_onRequestNewKey) ``` ...got reformatted to: ```haskell onNewKey <- performEvent $ createKey signingKeys <$> -- Filter out keys with empty names ffilter (/= "") (conf ^. walletCfg_onRequestNewKey) ```

lspitzner commented

2018-09-17 23:24:40 +02:00

(Migrated from github.com)

For the last two mentioned cases, issues #79 and #146 are very relevant. I will probably implement what @chreekat suggests in #146 via a new config option, although I still have to look into how finely grained this will be (and perhaps if there is a reason to change default config too, although that touches on a different topic..), also i think i am up to two unrelated issues that surface once you do that.. I will follow up on this in #146.

I think it is best to focus on these issues, as their resolution as i imagine it is in line with "visible screenspace density optimization", and probably resolves this part of this issue.

And even if it is a nice concept, an (admittedly brief) consideration does not let me belief that there are simple changes to the code possible to implement it. You could go encode a "density" attribute in the spacing information used in the internal algorithm, but that is a large-scope change, with questionable effectiveness - because supporting overflowing spacings would eliminate an important means for pruning the search space, so the result might be significantly less performant or yield worse results.

For the last two mentioned cases, issues #79 and #146 are very relevant. I will probably implement what @chreekat suggests in #146 via a new config option, although I still have to look into how finely grained this will be (and perhaps if there is a reason to change default config too, although that touches on a different topic..), also i think i am up to two unrelated issues that surface once you do that.. I will follow up on this in #146. I think it is best to focus on these issues, as their resolution as i imagine it is in line with "visible screenspace density optimization", and probably resolves this part of this issue. And even if it is a nice concept, an (admittedly brief) consideration does not let me belief that there are simple changes to the code possible to implement it. You could go encode a "density" attribute in the spacing information used in the internal algorithm, but that is a large-scope change, with questionable effectiveness - because supporting overflowing spacings would eliminate an important means for pruning the search space, so the result might be significantly less performant or yield worse results.

Sign in to join this conversation.

No Milestone

No project

No Assignees

1 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: hexagoxel/brittany#180