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.

Sign up for GitHub

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

Jump to bottom

Fix nested values inside anonymous records and types not displaying properly in stroustrup style #2687

Merged

nojaf merged 8 commits into fsprojects:main from josh-degraw:nested-anon-records

Jan 6, 2023

Contributor

josh-degraw commented Jan 4, 2023

Closes #2413

nojaf requested changes

View reviewed changes

Contributor

nojaf left a comment

Hi Josh, thank you so much for this PR. I really appreciate you working on this. Many people want Stroustrup and you are the only one making a real difference in this 🎉.

Praise aside, I'm afraid I negatively reviewed this PR.
Overall the required changes to pull this off are very scope to the junction of identifier, token (:/=) and body (expr/type). So I would really like to see only changes in those areas.

We don't apply the boy scout rule here. In a couple of months from now, something (which we didn't cover with a unit test, unfortunately) can break. When retracing our steps, it helps a bunch when the relevant PRs are small and focused.

That being said, I'm not against cleaning up the code. But if prefer it to happen as separate PRs (for example #2678), so we can easily detect if those changes introduced a problem or not.

I also noticed these changes don't cover the regular record instance:

[<Test>]
let ``regular records`` () =
    formatSourceString
        false
        """
let foo = {
    Data =
        {
            Name = "Isaac"
            Age = 43
            Day = "Monday"
            Colours =
                [
                    "Red"
                    "Blue"
                    "White"
                    "Orange"
                    "Red"
                    "Blue"
                    "White"
                    "Orange"
                    "Red"
                    "Blue"
                    "White"
                    "Orange"
                    "Red"
                    "Blue"
                    "White"
                    "Orange"
                ]
        }
}
"""
        config
    |> prepend newline
    |> should
        equal
        """
let foo = {
    Data = {
        Name = "Isaac"
        Age = 43
        Day = "Monday"
        Colours = [
            "Red"
            "Blue"
            "White"
            "Orange"
            "Red"
            "Blue"
            "White"
            "Orange"
            "Red"
            "Blue"
            "White"
            "Orange"
            "Red"
            "Blue"
            "White"
            "Orange"
        ]
    }
}
"""

it seems like a good opportunity to include those as well.

To move forward I would revert the changes to Fantomas.Core and reintroduce them one by one until the tests pass again.

src/Fantomas.Core/CodePrinter.fs Outdated

@@ @@ -3032,8 +3032,8 @@ let genType (t: Type) = @@
                           | None -> sepOpenAnonRecdFixed +> addSpaceIfSpaceAroundDelimiter
                           | Some n -> genSingleTextNode n +> addSpaceIfSpaceAroundDelimiter
-                      let genAnonRecordFieldType (i, t) =
-                          genSingleTextNode i
+                      let genAnonRecordFieldType (textNode, t) =

Contributor

nojaf Jan 5, 2023

Nitpick but i stood for identifier.
textNode no longer really carries that semantic.

Contributor Author

josh-degraw Jan 5, 2023

Fair enough, I had no idea what it was supposed to stand for so I assumed it was leftover as like an iterator var name and renamed it to indicate the type of the value. I can change it back if you'd like tho

Contributor

nojaf Jan 5, 2023

Maybe change it to identifier or identifierNode. textNode was a tad misleading.

src/Fantomas.Core/CodePrinter.fs Outdated

-                                  +> indentSepNlnUnindent (atCurrentColumnIndent genFields)
+                          let genAnonRecordFields = col sepNln node.Fields genAnonRecordFieldType
+                          handleBracketStyle (function

Contributor

nojaf Jan 5, 2023

I'm not really a fan of introducing a new helper function with a single usage.
Especially when this usage is essentially doing the same as ifAlignOrStroustrupBrackets.

In general, please try to reduce your PR to make the smallest amount of changes.
Before committing, circle back to everything and reflect on whether each change is really necessary to make your point.

Contributor Author

josh-degraw Jan 5, 2023 •

edited

Loading

Yeah sorry, originally I had used this in more than one place, and did slightly more than just match on the bracket type, but I guess I removed the other usage without realizing it haha

src/Fantomas.Core/CodePrinter.fs Outdated


		fun (ctx: Context) ->
		let size = getRecordSize ctx node.Fields

Contributor

nojaf Jan 5, 2023

Please also don't introduce these additional newlines.
Fantomas will combine two single lines of code without an additional new line.
That is the heuristic and we wish to respect that.

By adding the extra newline you took a stylistic decision that the previous developer on the same team didn't make. We use a code formatter to remove every personal touch any developer might make, so please keep an eye out for that.

Contributor Author

josh-degraw Jan 5, 2023

I'm not 100% sure if it's the case here but I noticed that when I ran the FormatAll build command it added a bunch of newlines for me. I thought I removed them but I guess not. Not sure why it would have done that

src/Fantomas.Core/CodePrinter.fs Outdated

		let multilineExpression (ctx: Context) =
		let noMembers = List.isEmpty members

Contributor

nojaf Jan 5, 2023

You shouldn't make any changes whatsoever to TypeDefn.Record in this PR.

Contributor Author

josh-degraw Jan 5, 2023

Sorry, leftover from trying to find the source of the issue 😅. Will change back

src/Fantomas.Core/CodePrinter.fs Outdated

@@ @@ -3458,7 +3458,10 @@ let genField (node: FieldNode) = @@
                       | Some name ->
                           genSingleTextNode name
                           +> sepColon
-                          +> autoIndentAndNlnIfExpressionExceedsPageWidth (genType node.Type))
+                          +> ifElseCtx

Contributor

nojaf Jan 5, 2023

You want to introduce a new function similar to autoIndentAndNlnExpressUnlessStroustrup but which takes a Type instead of an Expr. Accepting any single Type will eventually lead to invalid code.

Example:

type MangaDexAtHomeResponse = {
    baseUrl: string
    chapter: {|
            hash: string
            data: string[]
            otherThing: int
        |}
        ->
            // comment
            (int -> int) -> {|
            hash: string
            data: string[]
            otherThing: int
        |}
}

This is an extreme example of course but it would only be a matter of time before someone encounters some problem in the wild.

The type needs to be formatted like this:

            {|
                hash: string
                data: string[]
                otherThing: int
            |}
                ->
                // comment
                (int -> int)
                -> {|
                    hash: string
                    data: string[]
                    otherThing: int
                |}

in order to remain valid.

Contributor Author

josh-degraw commented Jan 5, 2023

We don't apply the boy scout rule here. In a couple of months from now, something (which we didn't cover with a unit test, unfortunately) can break. When retracing our steps, it helps a bunch when the relevant PRs are small and focused.

No worries, that's a habit I've gotten into after working on lots of legacy systems that notoriously never went back to address tech debt, but I can have a tendency to get a little carried away with that 😅 I know it's important to isolate changes as much as possible so no offense taken haha. I'll clean it up.

Contributor

nojaf commented Jan 5, 2023

Thanks 😊

josh-degraw force-pushed the nested-anon-records branch from 294bb4e to 60d83c9 Compare

January 5, 2023 16:38

Josh DeGraw added 6 commits

January 5, 2023 10:06


          nested anonymous record in value binding

b5d5656


          Fix issue with nested anonymous record type definitions

cbcbc08

Add test for list inside anonymous record type
Rename a value
Remove dumpAndContinue call


          Address PR comments

3016a90


          Revert pretty but unnecessary refactoring

2a87627


          Remove handleBracketStyle

8f5aeb7


          Fix nested record & collection stroustrup style issues

057804a

josh-degraw force-pushed the nested-anon-records branch from 980edb3 to 057804a Compare

January 5, 2023 17:08

Contributor Author

josh-degraw commented Jan 5, 2023

I also noticed these changes don't cover the regular record instance:
...
it seems like a good opportunity to include those as well.

Good catch, I didn't even realize those weren't already handled correctly! Got those fixed up.

nojaf added 2 commits

January 6, 2023 08:49


          Rename textNode to identifier.

c77c191


          Add regression test for 2587.

5ad9f2e

nojaf approved these changes

View reviewed changes

Contributor

nojaf left a comment

Thanks again for this PR!

nojaf merged commit 9d74413 into fsprojects:main

josh-degraw deleted the nested-anon-records branch

January 6, 2023 17:09

panmona mentioned this pull request

Stroustrup style breaks on nested records #2587

Closed

3 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet