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

Cursor with defines #2774

Merged
merged 7 commits into from
Mar 19, 2023
Merged

Cursor with defines #2774

merged 7 commits into from
Mar 19, 2023

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Feb 17, 2023

This is related to #2727 and tries to tackle the scenario where we format the code multiple times and merge each back into a single result.

For example:

let v =
  #if A
    'A'
  #else
    #if B
      'B'
    #else
      'C'
    #endif 
  #endif

Fantomas will format this three times:

  • []
  • [ "A" ]
  • [ "B" ]

In this PR, I'm trying to merge all the results in one go. In the main branch we are reducing the results instead, so I think for large files this will be quite beneficial in terms of performance.

@dawedawe and @josh-degraw could you please carefully review this PR? The goal was to make everything a bit more understandable than how it works today. I'm not quite sure I've hit the mark just yet.

There are some TODOs left, please let me know what you would do in those cases. I'm not quite sure about everything just yet.

Copy link
Member

@dawedawe dawedawe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my perspective it's definitely going in the right direction 👍

src/Fantomas.Core/MultipleDefineCombinations.fs Outdated Show resolved Hide resolved
src/Fantomas.Core/MultipleDefineCombinations.fs Outdated Show resolved Hide resolved
src/Fantomas.Core/MultipleDefineCombinations.fs Outdated Show resolved Hide resolved
src/Fantomas.Core/MultipleDefineCombinations.fs Outdated Show resolved Hide resolved
src/Fantomas.Core/MultipleDefineCombinations.fs Outdated Show resolved Hide resolved
src/Fantomas.Core/MultipleDefineCombinations.fs Outdated Show resolved Hide resolved
src/Fantomas.Core/MultipleDefineCombinations.fs Outdated Show resolved Hide resolved
src/Fantomas.Core/MultipleDefineCombinations.fs Outdated Show resolved Hide resolved
src/Fantomas.Core/MultipleDefineCombinations.fs Outdated Show resolved Hide resolved
@@ -4,7 +4,8 @@ module internal Fantomas.Core.CodeFormatterImpl
open FSharp.Compiler.Diagnostics
open FSharp.Compiler.Syntax
open FSharp.Compiler.Text
open Fantomas.Core.SyntaxOak
open SyntaxOak
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

open Statement not needed

Suggested change
open SyntaxOak

Comment on lines 38 to 39
override this.Equals _ = false
override this.GetHashCode() = Int32.MinValue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
override this.Equals _ = false
override this.GetHashCode() = Int32.MinValue
override _.Equals _ = false
override _.GetHashCode() = Int32.MinValue

@nojaf nojaf force-pushed the cursor-with-defines branch from a8986d4 to f58c7e5 Compare March 18, 2023 14:39
@nojaf nojaf merged commit 17deecc into fsprojects:v6.0 Mar 19, 2023
nojaf added a commit that referenced this pull request Mar 27, 2023
* Refactor processing of multiple defines into separate module.

* Improve correctness of the behavior.

* Fix remaining Util tests.

* Format MultipleDefineCombinations.fs

* Apply suggestions from code review

Co-authored-by: dawe <[email protected]>

* Remove redundant elif

* Implement Equals and GetHashCode.

---------

Co-authored-by: dawe <[email protected]>
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.

2 participants