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

Regression in alternative_long_member_definitions behaviour, or docs miss? #1666

Closed
rehael opened this issue Apr 20, 2021 · 5 comments · Fixed by #1669
Closed

Regression in alternative_long_member_definitions behaviour, or docs miss? #1666

rehael opened this issue Apr 20, 2021 · 5 comments · Fixed by #1669

Comments

@rehael
Copy link
Contributor

rehael commented Apr 20, 2021

Issue created from fantomas-online

Code

type C(aVeryLongType: AVeryLongTypeThatYouNeedToUse,
       aSecondVeryLongType: AVeryLongTypeThatYouNeedToUse,
       aThirdVeryLongType: AVeryLongTypeThatYouNeedToUse) =
    class
    end

Result

type C
    (
        aVeryLongType: AVeryLongTypeThatYouNeedToUse,
        aSecondVeryLongType: AVeryLongTypeThatYouNeedToUse,
        aThirdVeryLongType: AVeryLongTypeThatYouNeedToUse
    ) =
    class
    end

Problem description

Regression in fsharp_alternative_long_member_definitions – expected no change whatsoever, got alternate formatting. Flipping the setting doesn't change the parenthesis placement formatting.

Is it caused by #1334 missing updating the docs?

Anyway, extra question then – why tuples are not taken into consideration with MultilineBlockBracketsOnSameColumn?

Options

Fantomas Master at 04/17/2021 11:23:36 - 26674f9

Default Fantomas configuration

Did you know that you can ignore files when formatting from fantomas-tool or the FAKE targets by using a .fantomasignore file?

@nojaf
Copy link
Contributor

nojaf commented Apr 21, 2021

Hello, thank you for showing interest in this project.
The documentation is not up to date so it seems.
In short, Fantomas follows two style guides: the one from Microsoft by default and the one from G-Research via various settings.
These style guides evolve over time so the delta of the fsharp_alternative_long_member_definitions setting became smaller as the Microsoft guide was updated.

Are you interested in submitting a PR to update the documentation?

Anyway, extra question then – why tuples are not taken into consideration with MultilineBlockBracketsOnSameColumn?

MultilineBlockBracketsOnSameColumn was introduced to facilitate the G-Research style guide.

@rehael
Copy link
Contributor Author

rehael commented Apr 21, 2021

Sure, I can prepare a PR, maybe later today I will find some time. But first I need same clarity what should be the proper thing to do.

It looks like now the only difference this setting makes, is the placement of return type and = sign. Although I'm, not even sure about that, because G-Research style doesn't say anything explicitly about it. It looks like asking both style guides' authors what is their approach would have merit – because at then end of the day both teams might come to the same guidance. And then having this setting wouldn't have sense whatsoever.

What do you think?

@nojaf
Copy link
Contributor

nojaf commented Apr 21, 2021

Well, there is a bit of history and context to this. Originally the Microsoft guide had a different view on things and I believe it was changed in dotnet/docs#21690. The change was introduced to avoid "vanity alignments".

These things were discussed without having a clear view on the implementation side of things in Fantomas.
The style guide is not tied to Fantomas, nor was anybody involved from the G-Research style guide.
The conversation just kinda happened and they landed very close to what G-Research had all along.
So, this reflected in the setting being reduced to determining the location of the = sign.

Next, there is also something worth mentioning about the style guides. The Microsoft one is an open one. You can discuss and contribute to it via GitHub (there is a feedback button down below the page). The G-Research one is closed. They have published their guide to GitHub but it represents their vision.
So the only option, in this case, would be to pitch the G-Research style as a GitHub issue in the Microsoft one.
Perhaps it would fly, perhaps not. Ideally, it does and there is no need for a setting in Fantomas.

In case things stay as they are, the only thing that needs a change is the documentation in Fantomas.

To summarize, it sorta depends on how invested you are in all of this. You can start conversations on both sides, try and get the intent of why things are why they are and see how you can reconcile them.

rehael added a commit to rehael/fantomas that referenced this issue Apr 21, 2021
fix: AlternativeLongMemberDefinitions changed in fsprojects#1334 but were not reflected in documentation.

Closes: fsprojects#1666
@rehael
Copy link
Contributor Author

rehael commented Apr 21, 2021

For now, I created a PR with a docs fix.

Thank you for providing additional context! As for discussing further aligning the code style for constructors and member definitions with G-Research and Microsoft, then unless you would like to engage with them directly, I might open some GitHub issues to align that setting later this week.

nojaf pushed a commit that referenced this issue Apr 21, 2021
…1669)

fix: AlternativeLongMemberDefinitions changed in #1334 but were not reflected in documentation.

Closes: #1666
@nojaf
Copy link
Contributor

nojaf commented Apr 21, 2021

Feel free to engage with both guides yourself. Thank you for your efforts!

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.

2 participants