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

Proof of concept for short expression check. #721

Merged
merged 51 commits into from
Apr 13, 2020

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Mar 13, 2020

@jindraivanek this is a suggestion to approach formatting short expressions.
The main idea is that we try an expression similar to a dummy context but shortcut as soon as possible if we detect that the expression if too long or it is multiline.

let internal isShortExpression maxWidth (shortExpression: Context -> Context) (fallbackExpression) (ctx: Context) = ...

We can use the shortExpression writerEvents if it is indeed short, else we execute the fallback expression.
Please let me know what you think about this, and if you see any (major) performance concerns.

This PR is a draft, I would finish it once you validated the main concept.

Looping in @Smaug123 as well.


Update:

  • Record instances
  • Record definitions
  • Anounymous record instances
  • Anounymous record types?
  • Array/Lists
  • [ ] Revisit if/then/else
  • Infix operators
  • Tuples (no setting for this for now)
  • LetBindings
  • [ ] Remove IsDummy
  • Remove multliline

New settings:

  • MaxInfixOperatorExpression: Num
  • MaxRecordWidth: Num
  • MaxArrayOrListWidth: Num
  • MaxLetBindingWidth: Num

@nojaf nojaf requested a review from jindraivanek March 13, 2020 13:11
@nojaf
Copy link
Contributor Author

nojaf commented Mar 13, 2020

Related to issue #697

@nojaf
Copy link
Contributor Author

nojaf commented Mar 13, 2020

@jindraivanek the build fails because I didn't take any other unit tests into account, please only look at the four new ones to see the impact of the code change.

@jindraivanek
Copy link
Contributor

I like it, especially the idea of applying shortExpression only once. 👍

It looks like so similar to what Dummy is doing and shortcutting there also make sense, maybe idea of merging these modes into one (with more options) worth exploring?

@nojaf nojaf mentioned this pull request Mar 16, 2020
@nojaf
Copy link
Contributor Author

nojaf commented Apr 13, 2020

3260b63 and f00e496 remove the multiline function.

This was heavily used to determine if module declarations, let bindings inside let bindings, member bindings and member definitions are multiline.
If those constructs are multiline they should have a blank before and after.

For example:

let someBinding =
    let a = 42 // short

    let b =
         someFunctionCall a 8
         |> anotherFunctionCallMakingAllOfThisMultiline
         |> foo

   let c = "short"

In sepNlnBeforeMultilineConstruct detection is done if the previous construct already was multiline. To avoid double blank lines between a and b in case they are both long.
The detection is being done via the writer events in order to avoid future newline checks.

If a were to be multiline then it should not have a blank line between let someBinding =. This is an general exception that applies when the first construct is multiline.

One thing to notice is that trivia can sometimes make a construct multiline.

@nojaf
Copy link
Contributor Author

nojaf commented Apr 13, 2020

IsDummy cannot fully be replaced by the new approach. There are still a couple of places where you need to check first, make some decisions and move forward.
If/Then/Else is one of those areas. I might revisit this another time.

This PR is ready for review.
@jindraivanek please check the last commits, just in case.

@nojaf nojaf marked this pull request as ready for review April 13, 2020 18:51
@nojaf nojaf merged commit 1ef27b7 into fsprojects:master Apr 13, 2020
@nojaf nojaf deleted the feature/short-record-expression branch April 13, 2020 19:39
@drhumlen
Copy link

🎉

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.

None yet

3 participants