fsprojects / fantomas

FSharp source code formatter
https://fsprojects.github.io/fantomas
Other
773 stars 194 forks source link

Vertically align record and discriminated union type annotations #500

Open reinux opened 5 years ago

reinux commented 5 years ago

Description

It would be nice if record and DU fields could be vertically aligned, or at least kept vertically aligned if they already are, both in the type definition as well as in instantiation, if that's possible.

Currently,

type Person = {
    id          : ID
    name        : string
    age         : int
    nationality : Nation option
    likesPho    : bool
}

let rei = {
    id          = newID()
    name        = "rei"
    age         = 33
    nationality = Some (Nations.Japan)
    likesPho    = true
}

Becomes

type Person =
    { id : ID
      name : string
      age : int
      nationality : Nation option
      likesPho : bool }

let rei =
    { id = newID()
      name = "rei"
      age = 33
      nationality = Some(Nations.Japan)
      likesPho = true }

But it would be nice if it would retain the spacing, maybe keeping the = and : (and of for DUs) aligned with a space or tab after the longest name.

Something similar with DUs and pattern matching would be nice too, so that this spacing is maintained:

type WindowState<'model> =
  | Closed
  | Hidden  of 'model
  | Visible of 'model

let map (f: 'a -> 'b) (state: WindowState<'a>) =
  match state with
  | Closed    -> Closed
  | Hidden m  -> Hidden (f m)
  | Visible m -> Visible (f m)

I find this really improves readability, even though it can be a bit of a pain to do manually.

I suspect this could also apply to any situation where each line in, say, a list comprehension, contains a single expression with a repeating pattern of operators, but that's certainly not trivial to implement. Even just handling the cases where the operator is prescribed by the syntactical context would be awesome.

Repro code

Link

nojaf commented 4 years ago

Hey @reinux , this looks a nice suggestion. I should check how difficult it is to implement.

Tener commented 3 years ago

Hi @nojaf , have you managed to look into complexity of this? We have a codebase which looks much better with coding style enabled by this extension :)

nojaf commented 3 years ago

Hello, at the current time I have no interesting to pursue this. This actually falls under the category "What are we not looking for?". I'm stricter in these things these days. I hope you understand.

Tener commented 3 years ago

Thank you for the update @nojaf, I certainly understand this feature (however useful) may fall outside of the scope of the project as you see it. I'd love to hear why you made this decision, as it isn't clear from the link. Would it be too complex? Too niche? Too hard to implement correctly? Outside of the style guides?

Looking from another angle - would PR implementing such a feature has any chance of being merged?

In any case I'm very grateful for your work and the fact that this project exist; I'm sure it will prove very valuable tool in my own daily work as well as that of my team.

nojaf commented 3 years ago

It might not be that complex to implement but this project has bigger fish to fry. There are still a lot of open bugs that break people's source code. I want these to be resolved first that we have a stable base before introducing a whole set of cosmetic feature such as this one. I mean can you imagine you run a tool to clean things up and it just breaks all your code?

Another problem this feature introduces is that it is a suggestion of an individual. It is your typical "We format our code at home like this" request, Fantomas is too complex to be tailored to everybody's subtle differences. It is impossible to also measure what the impact of having this would be. That is why I limit all the stylistic requests to two style guides. Everything that does not fit into these guides, I just don't care anymore. That sounds harsh but I get requests on weekly basis by people who do not know the code base and have no interest in helping the besides their own little feature they want.

So yeah, even if you would submit a PR, it would not be accepted as it does not move the project further and it introduced a lick of extra complexity.

There is one thing you can try of course: get it in a style guide. The GR one is a closed system, but the one from Microsoft can be discussed or improved in the open. image

The this page button opens a GitHub issue where you could suggest the change in style.

All of that being said, when will Fantomas be stable enough? Well, my goal is to format the F# compiler itself. Once that is achieved I'll be open for these sorts of requests.

Tener commented 3 years ago

Thank you for the context, I believe I understand your point of view now. In fact, I think it is very sensible approach, even though it means this particular feature won't get implemented any time soon (if at all) :)

nojaf commented 5 months ago

As mentioned on our Discord, I've been using the Go formatter in recent weeks. There, go fmt will align the types in a struct:

image

There is some faint appeal to this. Subjectively it can be somewhat more readable. This, of course, has the to-be-expected git diff and tooling refactoring problems.

Anyway, the codebase these days is in a better state and I made a quick prototype. It is not impossible I'd say.

I'm going to reopen this and people can upvote this. With enough interest, we might be open to taking a PR for this. (Note, I personally don't care for this, so it won't be me 😉)

Tener commented 5 months ago

I believe that readability benefit comes from the columnar nature of the data. People know how to quickly read spreadsheets. Quite often you don't care about one column (names) or the other (types). This arrangement enables you to discard the irrelevant parts with much lower effort.

And yeah, as a daily go fmt user (these days), I definitely enjoy that part of its feature set.

Fxplorer commented 5 months ago

As a newbie and being very naive, this formatting is actually very appealing. Asking the idea from the previous perspective, would having this option as a separate package "add-in" or "plug-in" concept be very hard to do? I am certainly not knowledgeable enough with the code base to answer, but adding a step process from the "extension" seems like a logical way to do something like this.

nojaf commented 5 months ago

would having this option as a separate package "add-in" or "plug-in" concept be very hard to do?

Yes, having a plugin model would require a significant rewrite. And it would be challenging to guarantee plugins remain compatible between versions. Our AST model is unstable (both in F# compiler and Fantomas) and that is unlikely to ever change.

I wouldn't be opposed to the idea, I guess, but it would be a ton of work. I don't see anyone doing all that hard work to be fair 🙈.

realparadyne commented 5 months ago

I would like to see this feature as long as it made a check in each place to see if the rows (at least one? all? or more than half?) were already aligned or had extra spaces and then preserved that choice or intent.

For example, if I add a new unaligned row to an aligned definition and then format, my new row will also become aligned. But if I add an aligned row to a record where the rest are unaligned, my new row would be converted to unaligned to match the majority. And this would be per record / DU etc.

nojaf commented 5 months ago

That would lead to a mix of different styles which is the exact problem this project tries to solve.