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

Added clarification for match expression multiline case #40762

Closed
wants to merge 1 commit into from

Conversation

Lanayx
Copy link
Contributor

@Lanayx Lanayx commented May 5, 2024

Discussed and approved in fsharp/fslang-design#650


Internal previews

📄 File 🔗 Preview link
docs/fsharp/style-guide/formatting.md F# code formatting guidelines

@Lanayx Lanayx requested review from BillWagner, KathleenDollard and a team as code owners May 5, 2024 20:06
@dotnet-bot dotnet-bot added this to the May 2024 milestone May 5, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates PR is created by someone from the .NET community. label May 5, 2024
@@ -1092,16 +1092,24 @@ match l with
| [] -> failwith "Couldn't find David"
```

If the expression on the right of the pattern matching arrow is too large, move it to the following line, indented one step from the `match`/`|`.
If one of the expressions on the right of the pattern matching arrow is too large, move it to the following line, indented one step from the `match`/`|`. Move all the other expressions to the next line as well for consistency.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't interpret Don's last comment in the suggestion as setting this as the default, but rather an option.
I think the wording should reflect that (or the suggestion discussion continued about the default-ness of this)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dsyme set the label style-guide-decided, so I regard it as a marker that style guide is decided (final decision). As I understand, Don was not sure about tooling changes needed to support this style, but it should not affect the style and the recommendation itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The label is fine.
What I do not see confirmed is that "defaultness" of this, I interpret the context of the suggestion that this should still remain a possible option only.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, looks like it can't be resolved without Don

Copy link
Member

@abonie abonie May 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @T-Gro , here is what Don said

I agree with @isaacabraham that at option (4) listed above should be an option.

(emphasis mine)
cc: @vzarytovskii

@alandryz83

This comment was marked as off-topic.

@alandryz83

This comment was marked as off-topic.

@alandryz83

This comment was marked as off-topic.

@Lanayx
Copy link
Contributor Author

Lanayx commented May 7, 2024

Closing PR as it appeared that assumption of approved style was wrong

@Lanayx Lanayx closed this May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution Indicates PR is created by someone from the .NET community. dotnet-fsharp/svc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants