-
Notifications
You must be signed in to change notification settings - Fork 148
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
Should single-line if
expressions be allowed?
#209
Comments
I guess this reduces to "should single-line if-expressions be allowed?" I'd be fine with that. I've used them before, and they didn't seem particularly more prone to getting out of hand than any other kind of expression. |
I think this is worth considering. Though for the example given, I would tend to use isPalindrome list =
let
step ( x, y ) acc =
if x == y then
acc
else
False
in
List.map2 (,) list (List.reverse list)
|> List.foldl step True I'd love to see a few more examples from real code. Are inline lambdas the main place where this would be useful? |
I'm right now on a road of implementing 99 Haskell problems in Elm. And this is just a code for one of those problems. I would try to share more examples while moving through this problems. However I really like idea with |
Here's an example from real code: getAtPath configuration.path model.data
|> valueToString
|> (\str ->
if str == "Yes" ||
str == "No" then model.translate str else str
) turns to this: getAtPath configuration.path model.data
|> valueToString
|> (\str ->
if
str
== "Yes"
|| str
== "No"
then
model.translate str
else
str
) Definitely seems a bit too aggressive :) |
It looks like here is no problem with single/multiline if, but rather with boolean expression formatting. str
== "Yes"
|| str
== "No" This looks weird to me. |
I would definitely prefer if/then to be more flexible. |
I vote for allowing single-line toString points ++ " point" ++ (if points >= 2 then "s" else "") turned into toString points ++ " point"
++ (if points >= 2 then
"s"
else
""
) There is no specific syntax for ternary conditions in Elm (as in C) so let short |
Another vote for single-line if. It's much nicer in yesno b =
if b then "yes" else "no" seems nicer to read than yesno b =
if b then
"yes"
else
"no" |
Evan has requested that |
if
expressions be allowed?
@avh4 any update on this? The first thing I thought after I saw the blank-line-between-if-clauses in the 0.8.0 changelog was "I hope single-line ifs" are supported. |
I haven't had time to revisit this yet. If people have snippets of real-world code where they would use single-line if statements if available, please post them here! There have been a couple posted already, but I think it would be useful to see more examples of what this would look like in the wild. |
I just went through my codebase and pulled out some examples: After looking at a lot of I was surprised that single-line looked wrong much more often than I expected. That being said, I think there's still a case for leaving the choice up to the developer. If they leave it all on a single line, then let it be, similar to how function arguments are only pushed out to separate lines if the developer breaks it up into separate lines. |
Here's another real-world example I just found in my codebase... status quo: p
[ style
[ ( "width", "70%" )
, ( "padding", "6px" )
, ( "margin-left", "-6px" )
, ( "background-color"
, if isEndUser then
"#eef3ff"
else
"white"
)
]
]
[ text "..." ] single-line variant: p
[ style
[ ( "width", "70%" )
, ( "padding", "6px" )
, ( "margin-left", "-6px" )
, ( "background-color", if isEndUser then "#eef3ff" else "white" )
]
]
[ text "..." ] Single line seems like a much better choice here. |
Sorry to keep piling on, but I'm upgrading elm-json-tree-view for 0.19 and ran into this one while dealing with the multi-line: viewNodeInternal depth config node state =
case node.value of
TString str ->
viewScalar css.string ("\"" ++ str ++ "\"") node config
TFloat x ->
viewScalar css.number (String.fromFloat x) node config
TBool bool ->
viewScalar css.bool
(if bool then
"true"
else
"false"
)
node
config single-line: viewNodeInternal depth config node state =
case node.value of
TString str ->
viewScalar css.string ("\"" ++ str ++ "\"") node config
TFloat x ->
viewScalar css.number (String.fromFloat x) node config
TBool bool ->
viewScalar css.bool (if bool then "true" else "false") node config |
I’d love to see |
I had a possibly interesting experience around this. I also wanted oneline if-then-else since switching relatively short expression was somewhat common in my codebase, especially in views (e.g. just switching color or border style based on activation state.) But found elm-format does not allow that. In the mean time, let's just have this to avoid formatting: ite : Bool -> a -> a -> a
ite condition then_ else_ =
if condition then
then_
else
else_ ... and I used it casually, everywhere, everywhen I thought I wanted oneline logical switch. However, recently I realized that this helper function actually have a significant flaw: it eliminates laziness of if-then-else! It could be OK if both (To be clear: ite condition (funcitonForThen arg1) (functionForElse arg2) in this case both if condition then
functionForThen arg1
else
funcitonForElse arg2 With this, only one of them are evaluated!) So, allowing oneline if-then-else might have a practical "defensive" value in that it prevents someone like me from introducing such suboptimal escape hatch! |
@ymtszw yup, you probably want your function to have this type signature instead: its: Bool -> (() -> a) -> (() -> a) -> a
its c t e =
if c then
t ()
else
e () and then invoke it like ite c (\_ -> t) (\_ -> e) which is kind of a pain! |
An example with Current formatting el
[ paddingXY 15 30
, Border.color styles.colors.blue
, Border.widthEach
{ top = 0
, left = 0
, right = 0
, bottom =
if isSelected then
4
else
0
}
]
<|
text content Formatting I'd prefer el
[ paddingXY 15 30
, Border.color styles.colors.blue
, Border.widthEach
{ top = 0
, left = 0
, right = 0
, bottom = if isSelected then 4 else 0
}
]
<|
text content |
It has been almost three years now. Can we get a conclusion on this issue? |
My impression is that if somebody submits a pull request for this feature, it will most likely get merged.
|
Regarding the implementation: Here is the formatting done. elm-format/src/ElmFormat/Render/Box.hs Lines 1393 to 1424 in abe730a
Changing it to print in a single line should be easy. But I guess we want it to be context sensitive: If there are line breaks, they should stay, and if there are none, then it should be printed in single line. I think this information has to be collected when parsing and represented like it's done for other features (e.g. imports) |
Using Current, htmlAttribute <| style "transition" "all 0.5s ease-out"
, alpha <|
if isSelected then
1
else
0.6
, moveUp <|
if isSelected then
2
else
0 Should be, htmlAttribute <| style "transition" "all 0.5s ease-out"
, alpha <| if isSelected then 1 else 0.6
, moveUp <| if isSelected then 2 else 0 |
any updates on this? its driving me crazy... |
It seems there are enough real-world examples to justify testing an implementation, so I made one that only leads to single line expressions if there is no It seems to work quite well. The only drawback I noticed for now is that the following expression:
which was previously formatted as:
stays formatted as:
which might be a little confusing. Given their rarity (the elm-format test that uses one is the only example I ever saw), I'm not sure if nested if conditions should be handled differently or not (maybe there is a way to add parentheses in this example?). |
Inserting parens might indeed looks better. if (if a then True else False) then "a" else "b" However, as you probably noticed this should usually be reduced to: if a then "a" else "b" I wonder if there are practical cases where "nested -- BAD
if (if a then False else True) then "a" else "b"
-- GOOD
if not a then "a" else "b"
-- GOOD; with a function call
if not (resolvesToBool a) then "a" else "b" IMHO "nested Also as I wrote this, I started to wonder should longer conditional expressions be parenthesized? -- This:
if conditionalFunction (isActually quite complicated) andOrLong then "what should we do?" else "b"
-- Or this:
if (conditionalFunction (isActually quite complicated) andOrLong) then "what should we do?" else "b"
-- However I would prefer extracting such long conditional values to `let` bindings. My overall impression is: just leave them without parens.
|
Following code
turns to this
What do you think about it guys?
The text was updated successfully, but these errors were encountered: