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

Default values of width settings #2288

Closed
nojaf opened this issue Jun 3, 2022 · 6 comments · Fixed by #2364
Closed

Default values of width settings #2288

nojaf opened this issue Jun 3, 2022 · 6 comments · Fixed by #2364

Comments

@nojaf
Copy link
Contributor

nojaf commented Jun 3, 2022

As we are developing the next major version of Fantomas, it might be a good time to revisit the default values.

I believe there are three categories of settings:

  • Maximum width constraint settings
  • Settings that adhere to the G-Research style guide.
  • Auxiliary settings.

The first group is the one I'd like to revisit.
The second group should be disabled by default.
And the third group is probably fine as is.

On a side note, the maximum width settings are not mentioned in the style guide and I don't think this is a bad thing. Different projects will have different needs and some level of flexibility feels ok here.

Anyway, the default value of the following settings might need an update:

Please o please provide your reasoning for increasing or decreasing certain settings.
I personally find it terribly hard to change these default values because a few people say x or y "feels" better.
Changing these defaults will impact everyone using Fantomas, so "With great power, comes great responsibility"

@nojaf
Copy link
Contributor Author

nojaf commented Jul 8, 2022

@dsyme any thoughts on this given your experiences on formatting the compiler?

@dsyme
Copy link
Contributor

dsyme commented Jul 12, 2022

We're using this in the F# Compiler, though that's not necessarily representative

[*.fs]
max_line_length=140
fsharp_newline_between_type_definition_and_members=true
fsharp_max_function_binding_width=40
fsharp_max_if_then_else_short_width=60
fsharp_max_infix_operator_expression=80
fsharp_max_array_or_list_width=80
fsharp_max_array_or_list_number_of_items=5
fsharp_max_dot_get_expression_width=80
fsharp_multiline_block_brackets_on_same_column=true
fsharp_keep_max_number_of_blank_lines=1

[*.fsi]
fsharp_newline_between_type_definition_and_members=true
fsharp_keep_max_number_of_blank_lines=1

Of these I guess I'd argue for the following

  • fsharp_max_if_then_else_short_width=60 - F# has no ternary conditional operator so there are plenty of times you want single line if then else

  • fsharp_max_infix_operator_expression=80 - Possibly 60. Similar rationale as fsharp_max_if_then_else_short_width

  • fsharp_max_dot_get_expression_width=80 - Possibly 60. Similar rationale as fsharp_max_if_then_else_short_width

  • fsharp_newline_between_type_definition_and_members - this should be the default, in order to encourage XML docs

  • fsharp_max_array_or_list_width - Defining data using lists and arrays is common enough. We shouldn't penalise it resulting in sometimes 10s of extra lines.

  • fsharp_max_array_or_list_number_of_items - Similar rationale

I haven't yet played with ``fsharp_max_if_then_short_width`, I assume we'll want 0 like the default.

@nojaf
Copy link
Contributor Author

nojaf commented Jul 12, 2022

Thank you for your input, much appreciated. I think the rationale is on point here.

fsharp_max_infix_operator_expression=80 - Possibly 60

I think maybe 60 will be less impactful there. Doubling the existing size might shake things up too much.

fsharp_newline_between_type_definition_and_members - this should be the default, in order to encourage XML docs

Interestingly enough, this setting was implemented to adhere to the G-Research style guide.
If the MS guide has the same view on the topic I feel like we should remove the setting all together.

fsharp_max_array_or_list_width
I'm not sure what you mean there. Are you proposing 80 there as well?

I'll try these settings out on some code bases to see what this gives.

@auduchinok
Copy link
Contributor

fsharp_max_if_then_else_short_width=60 - F# has no ternary conditional operator so there are plenty of times you want single line if then else

This is one of the best examples where keeping the existing "single-line vs multi-line" style plus checking overall line length would likely work better in the most cases than checking an if expression length.

Tweaking the settings like this can mostly work for a single code base, but the formatter should work well on many projects, and tweaking settings like this will likely lead to aggressively rewriting of some perfectly-formatted code in some projects anyway.

In addition to that, if we want to enable incremental formatting during typing in future, we definitely don't want to rewrite code that aggressively, otherwise it wouldn't be a good user experience, so the formatter has to keep properly formatted code untouched.

It doesn't seem difficult to check if an if expression is one of the following forms (the tree has correct ranges for all of them):

if a then b else c
if a then
    b
else
    c
if a then b else

c

@dsyme
Copy link
Contributor

dsyme commented Jul 12, 2022

@nojaf asked for some more info

This is too much penalty for defining data:
https://fsprojects.github.io/fantomas-tools/#/fantomas/preview?data=N4KABGBEDGD2AmBTSAuKAbRAXMB[…]wOkaYyoXBF6WNmfRLA4wEDAj8dQgJVg9AVDCNYQkQ03CX3qEAA

I think this is only relevant to fsharp_max_array_or_list_width as it looks like fsharp_max_array_or_list_number_of_items is being ignored in my settings because I'm not using fsharp_array_or_list_multiline_formatter

But in any case the point I'm making is that data definitions can easily get penalized from 1 line to 10s of lines under default settings.

@nojaf
Copy link
Contributor Author

nojaf commented Jul 12, 2022

Hey @auduchinok, could you open a new issue for your points.
Happy to discuss this over there.

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

Successfully merging a pull request may close this issue.

3 participants