wider default fsharp_max_value_binding_width, also take width of pattern of binding into account #1936

dsyme · 2021-10-31T16:47:54Z

Adjusts the default fsharp_max_value_binding_width up to 80, see #1933

I adjusted things computation to take into account the width of the pattern of the let (pattern = expr), see comment below and adjustment to docs. This feels right to me.

@nojaf It feels right to make this change

@Smaug123 Could you find what G-Research default setting is? May make sense to simply go to that?

* Initial upgrade to FCS 40. * Fix remaining unit tests. * Remove AST workaround for Attributes. * Update for SynTyparDecls, SynPat.As and ParsedHashDirectiveArgument. * Re-enable ignored tests. * Update to FCS 40.0.1-preview.21366.4

* WIP lambda as last argument * Update remaining test according to style proposal. * Add test for multiline non lambda argument. * Print trivia for function keyword from ast range. * multiline non lambda argument, match lambda

* Update records indent from the curly brace. Fixes fsprojects#1876. * WIP anonymous records * Indent multiline record field expressions from the opening brace. * Restore correct indent after update record expression. * trigger CI

* Style guide lambda (fsprojects#1858) * WIP lambda as last argument * Update remaining test according to style proposal. * Add test for multiline non lambda argument. * Print trivia for function keyword from ast range. * multiline non lambda argument, match lambda * WIP Daemon service contract. * Early working daemon prototype. * Take .editorconfig settings into account. * Fix current build. * Add Fantomas locator. * Found globally installed Fantomas version. * Remove unused function. * Add formatSelection. * Refactor FormatDocumentResponse to DU. * Remove unused LspTypes dependency. * Update FCS in Daemon * Refactored public api of FantomasService. * Don't pass full exception in FormatDocumentResponse. * Use separate versioning for Fantomas.Client. * FantomasService manages multiple daemon versions. * Fully implement FantomasService. * Return configuration as json. * Validate if filePath in request is absolute. * Ignore first three 4.6 alphas as compatible tools. * Find ignore file from current file path. * Remove safeFileName from CodeFormatterImpl.fs. Disable daemon tests. * Add daemon unit tests. * Update help text for daemon mode. * Add documentation for Fantomas daemon mode. * Initial attempt at adding source links. * Set DebugType to embedded. * Bump Fantomas.Client to 0.3.0

…a. (fsprojects#1923) Fixes fsprojects#1922.

…fsprojects#1925) * Set DOTNET_CLI_UI_LANGUAGE to en-us when running dotnet tool command. * Add RepositoryUrl. * Bump Fantomas.Client version to 0.3.1

* Update FCS to 41.0.0-preview.21472.3 * If, elif, then and else keyword are present in AST. * Use arrow range from SyntaxTree instead of trivia tokens. * Processed change of FCS 40.0.1-preview.21423.4. * Support expr[idx] as index/slice syntax. * Remove ArrayOrListComputedExpr active pattern. * Apply style guide rules for SynExpr.IndexRange. * Always use LangVersionText preview. * Remove Verbose tests. * Change QuoteIdentifierIfNeeded to AddBackticksToIdentifierIfNeeded.

dsyme · 2021-10-31T17:33:39Z

@nojaf @Smaug123 I adjusted this to include the pattern in "let pattern = expr" in determining whether "fsharp_max_value_binding_width" applies. This feels right, e.g.

let aaaa, bbbb, cccc = expr

will split to

let aaaa, bbbb, cccc =
    expr

but

let a = expr

will not (for suitable length of expr etc.)

dsyme · 2021-10-31T17:55:45Z

@nojaf This is ready for review.

nojaf · 2021-10-31T19:15:06Z

Hey Don, sorry for this but would you mind targeting the 4.6 branch for this PR?
There was no way for you to know this, my bad.

The idea is that bug fixes are made against master and revisions are published from there.
For new features or grander stylistic changes, the feature branch should be used.
I'll add this to the contribution guidelines next week.
Sorry again, I hope you can cherry-pick the commits easily.

dsyme · 2021-10-31T21:06:50Z

Hey Don, sorry for this but would you mind targeting the 4.6 branch for this PR?

No probs, done. Do you manually merge master --> 4.6?

FWIW this is the opposite to dotnet/fsharp. There we do all feature work in master, and set up codeflows

master    // for feature work and bug fixes 
release/4.6   // this is the "next" release. 
release/4.5   // this is a previous release  
release/4.4   // this is a previous release

Code flow prior to releasing 4.6:

master --> release/4.6   // this is the "next" release. feature work and bug fixes flow to next release unless targeted at existing release
master <-- release/4.5   // bug fixes can be made in previous releases and will flow to master and future release

Code flow after releasing 4.6:

master --> release/4.7   // this is the "next" release. 
master <-- release/4.6   // bug fixes can be made in previous release and will flow to master and future release

The code flow is automated and creates integration PRs. I really wish github did this by default for all projects, and you could put a ".github/codeflow/config.yml" for active flows.

Smaug123 · 2021-10-31T22:25:18Z

Nothing is set in stone here as far as G-Research is concerned; due to coordination difficulties and the lack of time to resolve them, the things we've got formatted internally are sufficiently low-traffic that we would happily reformat them if it meant standardising on a "better" model of the world. I think this kind of line-length optimisation is probably one of the things we have the least strong opinions on.

nojaf · 2021-11-01T08:50:46Z

No probs, done. Do you manually merge master --> 4.6?

Thanks, yes I admit, I do this manually right now. 4.6 is the first version where we actually try and maintain two branches.
In the past, we did everything on master and from the first new unstable feature or FCS upgrade we labelled it alpha. Fixes went into the alpha but people were hesitant to try out these versions.

Now, we have released five revisions (4.5.X) and we do see a spike in downloads on NuGet.
People are more comfortable with this model.
Which, is a long way of telling that we are still figuring this out.
Thanks for the ".github/codeflow/config.yml", I'll definitely check this out sometime.

nojaf · 2021-11-01T08:52:58Z

fantomas.sln

 MinimumVisualStudioVersion = 15.0.26124.0
 Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = ".paket", ".paket", "{B570C54C-2FEA-4A94-A8C5-FD4157A94DE7}"
 	ProjectSection(SolutionItems) = preProject
 		paket.dependencies = paket.dependencies
 	EndProjectSection
 EndProject
-Project("{F2A71F9B-5D33-465A-A702-920D77279786}") = "Fantomas", "src\Fantomas\Fantomas.fsproj", "{7EA16279-A5F1-4781-A343-4375A04BCE6F}"
+Project("{6EC3EE1D-3C4E-46DD-8F32-0CC8E7565705}") = "Fantomas", "src\Fantomas\Fantomas.fsproj", "{7EA16279-A5F1-4781-A343-4375A04BCE6F}"


What does this mean? Or did this happen automatically by VS?

nojaf · 2021-11-01T08:55:16Z

src/Fantomas/CodePrinter.fs

@@ -4953,6 +4953,13 @@ and genMemberDefn astContext node =
                | None -> sprintf "abstract %s" s
            |> fun s -> !- s ctx

+        let hasGenerics =


Could you try and avoid having a PR that does two unrelated things?
I'm ok with this change, but it seems unrelated to changing the default settings.

It is the same fix as #1935, right?

nojaf · 2021-11-01T09:00:05Z

docs/Documentation.md

@@ -729,7 +730,7 @@ type MyType() =

 ### fsharp_max_function_binding_width

-Control the maximum width for which let and member function bindings should be in one line.
+Control the maximum width for which function and member bindings should be in one line.


Member bindings will look at two settings currently.
Example.

nojaf · 2021-11-03T08:07:22Z

Closing in favour of #1947. Thanks again Don.

nojaf added 11 commits October 27, 2021 22:23

Upgrade to FCS 40 (fsprojects#1856)

a193424

* Initial upgrade to FCS 40. * Fix remaining unit tests. * Remove AST workaround for Attributes. * Update for SynTyparDecls, SynPat.As and ParsedHashDirectiveArgument. * Re-enable ignored tests. * Update to FCS 40.0.1-preview.21366.4

Add release notes for 4.6.0-alpha-001.

c1a97df

Style guide lambda (fsprojects#1858)

daaf40c

* WIP lambda as last argument * Update remaining test according to style proposal. * Add test for multiline non lambda argument. * Print trivia for function keyword from ast range. * multiline non lambda argument, match lambda

Add release notes for 4.6.0-alpha-002.

2decf2c

Add release notes for 4.6.0-alpha-003.

3fda5d4

Don't add a newline if there are no additional arguments before lambd…

b7de991

…a. (fsprojects#1923) Fixes fsprojects#1922.

Set DOTNET_CLI_UI_LANGUAGE to en-us when running dotnet tool command. (…

33ed998

…fsprojects#1925) * Set DOTNET_CLI_UI_LANGUAGE to en-us when running dotnet tool command. * Add RepositoryUrl. * Bump Fantomas.Client version to 0.3.1

Add release notes for 4.6.0-alpha-005.

e4a18aa

dsyme changed the title ~~trial wider default fsharp_max_value_binding_width~~ wider default fsharp_max_value_binding_width, also take "let" pattern into account Oct 31, 2021

dsyme changed the title ~~wider default fsharp_max_value_binding_width, also take "let" pattern into account~~ wider default fsharp_max_value_binding_width, also take pattern of binding into account Oct 31, 2021

dsyme changed the title ~~wider default fsharp_max_value_binding_width, also take pattern of binding into account~~ wider default fsharp_max_value_binding_width, also take width of pattern of binding into account Oct 31, 2021

dsyme changed the base branch from master to 4.6 October 31, 2021 20:40

integrate to 4.6!

d460b53

dsyme force-pushed the wider branch from f60476f to d460b53 Compare October 31, 2021 21:00

nojaf reviewed Nov 1, 2021

View reviewed changes

nojaf force-pushed the 4.6 branch from db2a51f to e96a854 Compare November 2, 2021 15:43

nojaf mentioned this pull request Nov 3, 2021

wider default fsharp_max_value_binding_width #1947

Merged

nojaf closed this Nov 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wider default fsharp_max_value_binding_width, also take width of pattern of binding into account #1936

wider default fsharp_max_value_binding_width, also take width of pattern of binding into account #1936

dsyme commented Oct 31, 2021 •

edited

Loading

dsyme commented Oct 31, 2021 •

edited

Loading

dsyme commented Oct 31, 2021

nojaf commented Oct 31, 2021

dsyme commented Oct 31, 2021

Smaug123 commented Oct 31, 2021

nojaf commented Nov 1, 2021

nojaf Nov 1, 2021

nojaf Nov 1, 2021

nojaf Nov 1, 2021

nojaf Nov 1, 2021

nojaf commented Nov 3, 2021

wider default fsharp_max_value_binding_width, also take width of pattern of binding into account #1936

wider default fsharp_max_value_binding_width, also take width of pattern of binding into account #1936

Conversation

dsyme commented Oct 31, 2021 • edited Loading

dsyme commented Oct 31, 2021 • edited Loading

dsyme commented Oct 31, 2021

nojaf commented Oct 31, 2021

dsyme commented Oct 31, 2021

Smaug123 commented Oct 31, 2021

nojaf commented Nov 1, 2021

nojaf Nov 1, 2021

Choose a reason for hiding this comment

nojaf Nov 1, 2021

Choose a reason for hiding this comment

nojaf Nov 1, 2021

Choose a reason for hiding this comment

nojaf Nov 1, 2021

Choose a reason for hiding this comment

nojaf commented Nov 3, 2021

dsyme commented Oct 31, 2021 •

edited

Loading

dsyme commented Oct 31, 2021 •

edited

Loading