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

fsharp_experimental_stroustrup_style=true breaks on types with nested anonymous records #2413

Closed
2 of 3 tasks
LiteracyFanatic opened this issue Aug 6, 2022 · 8 comments · Fixed by #2687
Closed
2 of 3 tasks

Comments

@LiteracyFanatic
Copy link

Issue created from fantomas-online

Code

namespace MangaSharp.Extractors.MangaDex

open System
open System.Net.Http
open System.Net.Http.Json
open FsToolkit.ErrorHandling

type MangaDexAtHomeResponse = {
    baseUrl: string
    chapter: {|
        hash: string
        data: string[]
    |}
}

Result

namespace MangaSharp.Extractors.MangaDex

open System
open System.Net.Http
open System.Net.Http.Json
open FsToolkit.ErrorHandling

type MangaDexAtHomeResponse =
{ baseUrl: string
  chapter: {| hash: string; data: string[] |} }

Problem description

I often use types with nested anonymous records when modeling JSON responses from external APIs. The new fsharp_experimental_stroustrup_style setting seems to break on such type declarations.

Extra information

  • The formatted result breaks my code.
  • The formatted result gives compiler warnings.
  • I or my company would be willing to help fix this.

Options

Fantomas master branch at 2022-08-06T07:12:17Z - c1d5250

    { config with
                ExperimentalStroustrupStyle = true }

Did you know that you can ignore files when formatting from fantomas-tool or the FAKE targets by using a .fantomasignore file?

@nojaf
Copy link
Contributor

nojaf commented Aug 8, 2022

Hello,

A couple of things here.

Using fsharp_experimental_stroustrup_style without fsharp_multiline_block_brackets_on_same_column does not make any sense. ExperimentalStroustrupStyle is an extension of MultilineBlockBracketsOnSameColumn.

Enabling both will lead to valid code:

type MangaDexAtHomeResponse = {
    baseUrl: string
    chapter: {| hash: string //
                data: string[] |}
}

sample

However, and this might be the point you are trying to make, the anonymous type does not appear in the stroustrup style.
You would need to fix #2412 first because there is currently no implementation for MultilineBlockBracketsOnSameColumn for anonymous types.
Once that is in play, another change would be required in record field declarations of the record type definition.

Are you interested in submitting a PR for this?

@isaacabraham
Copy link

Just to add onto this, I'm seeing this with both stroustrup and multiline block brackets same column.

let foo = {| Data = {| Name = "Isaac"; Age = 43; Day = "Monday"; Colour = "Blue" |} |}

goes to

let foo =
    {| Data =
        {| Name = "Isaac"
           Age = 43
           Day = "Monday"
           Colour = "Blue" |} |}

rather than what I would like to see:

let foo = {|
    Data = {|
        Name = "Isaac"
        Age = 43
        Day = "Monday"
        Colour = "Blue"
    |}
|}

This is important for us on Farmer because we create very deeply nested anonymous records which represent Azure ARM JSON. Without this setting, the files go wide very quickly (although the Stroustrup setting does help otherwise e.g. lists etc.). Once you get to 120 characters width, Fantomas starts to (understandable) wrap text very quickly, leading to files that are very long yet lines that are 95% white space and just a few symbols at the very end.

@isaacabraham
Copy link

isaacabraham commented Jan 4, 2023

Confusingly, whilst inside an anonymous record, even lists seem to stop obeying the Stroustrup style.

Apologies - the link here doesn't seem to include settings (or the one in the post immediately above). Both are set in the Fantomas web app to 5.x, Multiline Block Brackets on Same Column = true and Experimental Stroustrup Style = true. Nothing else changed.

@nojaf
Copy link
Contributor

nojaf commented Jan 4, 2023

Hello,

We recently changed the online tool, to enable Stroustrup:
image

This is important for us on Farmer

Important enough to send a PR 😉?

@isaacabraham
Copy link

isaacabraham commented Jan 11, 2023

@josh-degraw thanks for fixing this :-)

@nojaf Apologies... Farmer suffers from this enough that I'm happy to contribute to an issue and give minimal repros. But I simply don't have the time to contribute more to this project, as much as I would love to :-(

@isaacabraham
Copy link

We recently changed the online tool, to enable Stroustrup

Is this change cascaded into editorconfig or are the old two settings still valid?

@josh-degraw
Copy link
Contributor

josh-degraw commented Jan 11, 2023

I made sure in #2658 to keep backwards compatibility, so the old settings should still work, but there is a new editorconfig setting that should hopefully be less confusing :)

@isaacabraham
Copy link

I'm still on 5.1.x anyway. I've now found the release notes and that explains all the changes. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants