DanielGavin / ols

Language server for Odin
MIT License
382 stars 58 forks source link

New line inside aggregate initialization, what is desired behavior? #298

Closed leidegre closed 6 months ago

leidegre commented 6 months ago

So, I have this struct

swap_chain_desc := dxgi.SWAP_CHAIN_DESC1 {
    BufferCount = BACK_BUFFER_COUNT,
    Width = 0,
    Height = 0,
    Format = dxgi.FORMAT.R8G8B8A8_UNORM,
    Flags = {.FRAME_LATENCY_WAITABLE_OBJECT},
    BufferUsage = {.RENDER_TARGET_OUTPUT},
    SampleDesc = {Count = 1, Quality = 0},
    SwapEffect = .FLIP_DISCARD,
    AlphaMode = .UNSPECIFIED,
    Scaling = .STRETCH,
    Stereo = false,
}

I would like it to be formatted like this

swap_chain_desc := dxgi.SWAP_CHAIN_DESC1 {
    BufferCount = BACK_BUFFER_COUNT,
    Width = 0,
    Height = 0,
    Format = dxgi.FORMAT.R8G8B8A8_UNORM,
    Flags = {.FRAME_LATENCY_WAITABLE_OBJECT},
    BufferUsage = {.RENDER_TARGET_OUTPUT},
    SampleDesc = {
        Count = 1, // see here, the Count and Quality is on a new line 
        Quality = 0,
    },
    SwapEffect = .FLIP_DISCARD,
    AlphaMode = .UNSPECIFIED,
    Scaling = .STRETCH,
    Stereo = false,
}

This might not always be warranted but I think it is possible to disambiguate these two, for example

If you want a one liner, you simply don't use the last comma

swap_chain_desc := dxgi.SWAP_CHAIN_DESC1 {
    SampleDesc = {
        Count = 1,
        Quality = 0, // remove this comma
    },
}

And you get the following

swap_chain_desc := dxgi.SWAP_CHAIN_DESC1 {
    SampleDesc = {Count = 1, Quality = 0},
}

@DanielGavin let me know if I can help out with any of this, I'm just making notes as I run into things. I'm very new to Odin but enjoy the language a lot.

DanielGavin commented 6 months ago

The problem is the formatter is written much in the vein of javascripts prettier/rustfmt. There is no way of telling the formatter it should use one line it. It uses the width of lines to determine whether it should be broken.

You could technically ignore the core concept of the formatter by having some break character like the comma, but what happens in the reverse, the user wants to one line it, but the character width is above the threshold? One could say well it should just look at the comma, but then the whole width line breaking is completly removed.

In that case it would be better just to write a whole new formatter that acts less opinionated like gofmt. That is not necessarily a bad idea. This formatter could potentially be too agressive for some people. Even I sometimes wonder whether making a lighter gofmt styled formatter would be better.

leidegre commented 6 months ago

As a language designer, I think having an "official" opinionated style is a good thing because it helps with readability. It's good that all the code looks the same.

As a programmer, I rather be programming and not care about formatting. If the choice is taken away from me it's one less thing to care about.

I was thinking that a comma could be useful as a hint but I get that if it runs into with width limit and that then triggers other behavior then it's just more confusing.

I made a quick test in Go and it doesn't seem to care about the comma, I can break where I like. It will just indent stuff.

Which would enable layouts like this but why would you wanna do that? At least, to me it strikes me as odd because it is inconsistent. IMAO easy to misread?

swap_chain_desc := dxgi.SWAP_CHAIN_DESC1 {
    BufferCount = BACK_BUFFER_COUNT,
    Width = 0, Height = 0,
    Format = dxgi.FORMAT.R8G8B8A8_UNORM,
    Flags = {.FRAME_LATENCY_WAITABLE_OBJECT},
    BufferUsage = {.RENDER_TARGET_OUTPUT},
    SampleDesc = {
        Count = 1,
        Quality = 0,
    },
    SwapEffect = .FLIP_DISCARD, AlphaMode = .UNSPECIFIED, Scaling = .STRETCH, Stereo = false,
}

And what about alignment, is this better, worse or doesn't matter kind of deal?

swap_chain_desc := dxgi.SWAP_CHAIN_DESC1 {
    BufferCount = BACK_BUFFER_COUNT,
    Width       = 0, 
    Height      = 0,
    Format      = dxgi.FORMAT.R8G8B8A8_UNORM,
    Flags       = {.FRAME_LATENCY_WAITABLE_OBJECT},
    BufferUsage = {.RENDER_TARGET_OUTPUT},
    SampleDesc  = {
        Count   = 1,
        Quality = 0,
    },
    SwapEffect = .FLIP_DISCARD, 
    AlphaMode  = .UNSPECIFIED, 
    Scaling    = .STRETCH, 
    Stereo     = false,
}

SampleDesc kinda stands out in the above example.

Also, I might have misunderstood the relevance of the comma here. It is required for the code to be syntactically valid. I don't even think it's possible to use as I originally thought.

The more I think about it I don't know that it makes anything simpler. Bah, forget what I said.