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

Fantomas 6 #1027

Closed
wants to merge 2 commits into from
Closed

Fantomas 6 #1027

wants to merge 2 commits into from

Conversation

nojaf
Copy link

@nojaf nojaf commented Mar 28, 2023

The changes in this PR are as follows:

  • Update the fantomas dotnet tool to v6.0.0-alpha-008 (See blogpost)
  • Format all the code

I have read the contributing guidelines and have completed the following:

  • Tested my code end-to-end against a live Azure subscription.
  • Updated the documentation in the docs folder for the affected changes.
  • Written unit tests against the modified code that I have made.
  • Updated the release notes with a new entry for this PR.
  • Checked the coding standards outlined in the contributions guide and ensured my code adheres to them.

Hi there,

We are trying to get some in-the-field feedback on our next major release.
If memory serves correctly someone was interested in the Stroustrup style for Farmer as it could be a good fit when there is a lot of configuration. I could be wrong and this was more about when using Farmer versus the actual Farmer codebase.
V6 has made a lot of improvements in that area, so I'm curious to hear your thoughts.

@nojaf
Copy link
Author

nojaf commented Mar 28, 2023

Still found a minor issue: fsprojects/fantomas#2806

@isaacabraham
Copy link
Member

Thanks so much - this is definitely highly desired - the stroustrup style makes life a lot easier, particularly in the Arm namespace because we make heavy use of nested list comprehensions, and without stroustrup it pushes everything across more and more.

@nojaf
Copy link
Author

nojaf commented Mar 30, 2023

Thanks for confirming, I'll get back to you later this week once we have fixed the found issue.

@isaacabraham
Copy link
Member

Would also be great to have fsharp_newline_before_multiline_computation_expression = true set as well.

(As an aside, isn't the setting named backwards? Surely it should be false for e.g

let x = foo {
    bar 123
}

@nojaf
Copy link
Author

nojaf commented Mar 30, 2023

Yeah, naming is hard 😅.

fsharp_newline_before_multiline_computation_expression = true by default. See https://fsprojects.github.io/fantomas/docs/end-users/Configuration.html#fsharp_newline_before_multiline_computation_expression

What we are going for:

There will be a newline before the computation expression by default.

    let something = // Here is the newline before the computation expression
        task {
            let! thing = otherThing ()
            return 5
        }

If you don't want the CE to go to the next line (aka you don't want the newline):

let something = (* no newline before the CE *) task {
    let! thing = otherThing ()
    return 5
}

So, you want the setting to be false?

@martinbryant
Copy link
Contributor

martinbryant commented Aug 18, 2023

Hi @nojaf - can we look at getting the conflicts dealt with? Thanks

@nojaf
Copy link
Author

nojaf commented Aug 18, 2023

@martinbryant done!

@martinbryant
Copy link
Contributor

Amazing @nojaf thanks :)

@ninjarobot
Copy link
Collaborator

Since we don't have any active PRs right now, I recommend we get this merged as soon as possible.

@nojaf can you please resolve the conflicts one more time?

@ninjarobot
Copy link
Collaborator

Or should this use fantomas 6.2.0 instead of 6.1.2 @nojaf ?

@nojaf
Copy link
Author

nojaf commented Sep 19, 2023

You can check out the version differences at https://github.com/fsprojects/fantomas/blob/main/CHANGELOG.md.

I'll go ahead and close this PR. Whether this gets merged or not doesn't really matter to me anymore. If someone else ever wants to do the upgrade, they can use this PR as a reference. 😊

@nojaf nojaf closed this Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants