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

fix up indentation in formatting guide #18793

Merged
merged 4 commits into from
Jul 12, 2020
Merged

fix up indentation in formatting guide #18793

merged 4 commits into from
Jul 12, 2020

Conversation

cartermp
Copy link
Contributor

@cartermp cartermp commented Jun 4, 2020

This was mistakenly suggesting something that won't work. This now uses the formatting that we do in the F# compiler for our own very very long member definitions (sometimes). Fixes #18637

This was mistakenly suggesting something that won't work. This now uses the formatting that we do in the F# compiler for our own very very long member definitions (sometimes). Fixes #18637
@dotnet-bot dotnet-bot added this to the June 2020 milestone Jun 4, 2020
@knocte
Copy link
Contributor

knocte commented Jun 8, 2020

I feel like the ReturnType is too much on the right here:

    member _.LongMethodWithLotsOfParameters(aVeryLongType: AVeryLongTypeThatYouNeedToUse,
                                            aSecondVeryLongType: AVeryLongTypeThatYouNeedToUse,
                                            aThirdVeryLongType: AVeryLongTypeThatYouNeedToUse): ReturnType =

Shouldn't we split that on to a new line like this?:

    member _.LongMethodWithLotsOfParameters(aVeryLongType: AVeryLongTypeThatYouNeedToUse,
                                            aSecondVeryLongType: AVeryLongTypeThatYouNeedToUse,
                                            aThirdVeryLongType: AVeryLongTypeThatYouNeedToUse)
                                                : ReturnType =

@cartermp
Copy link
Contributor Author

cartermp commented Jun 8, 2020

Sure, seems reasonable

@cartermp
Copy link
Contributor Author

cartermp commented Jul 9, 2020

@gewarren @IEvangelist can we get this one merged?

Copy link
Contributor

@gewarren gewarren left a comment

Choose a reason for hiding this comment

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

One spelling mistake to fix and suggest adding a comma too.

docs/fsharp/style-guide/formatting.md Outdated Show resolved Hide resolved
Copy link
Member

@IEvangelist IEvangelist left a comment

Choose a reason for hiding this comment

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

Looks good to me, we'll :shipit: - thank you 🙏

Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com>
@cartermp cartermp merged commit c978290 into master Jul 12, 2020
@cartermp cartermp deleted the cartermp-patch-1 branch July 12, 2020 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Place parameters on a new line for long member definitions advice can lead to compiler warnings
6 participants