-
-
Notifications
You must be signed in to change notification settings - Fork 193
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
Keep Subsequent identifier links on the same line. #2713
Conversation
""" | ||
|
||
[<Test>] | ||
let ``very long chain with a some index expressions`` () = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dawedawe this one is a bit tricky. The whole chain does not fit within the MaxDotGetExpressionWidth
boundary, but there is no application to chop it logically.
@@ -1123,7 +1129,7 @@ let genExpr (e: Expr) = | |||
short | |||
(match leadingChain with | |||
| [] -> sepNone | |||
| head :: links -> genFirstLinkAndIndentOther head links) | |||
| head :: links -> genLink false head +> indent +> genIndentedLinks true links +> unindent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dawedawe I changed the implementation to only consider the max_line_length
when there are only simple links in the chain.
You basically print the first one, if the second fits after it, great, if not put it on the next line. I think this is reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, very reasonable.
@@ -1123,7 +1129,7 @@ let genExpr (e: Expr) = | |||
short | |||
(match leadingChain with | |||
| [] -> sepNone | |||
| head :: links -> genFirstLinkAndIndentOther head links) | |||
| head :: links -> genLink false head +> indent +> genIndentedLinks true links +> unindent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, very reasonable.
Co-authored-by: dawe <[email protected]>
Fixes #2712.