Skip to content

[Cosmetic] Refactorings & Fantomas integration #157

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

Merged
merged 15 commits into from
Jul 19, 2020

Conversation

CaptnCodr
Copy link
Member

This PR includes:

  • Resolved warnings: especially type warnings list<_>, array<_>, seq<_> were removed twice, it runs under IEnumerable too
    • CollectionContainsConstraint was marked as obsolete: it's now Has.Some.EqualTo as recommended
  • Several indentation alignments in CustomMatchers.fs: 4x tabs (4 spaces) in function body
  • Removed redundant code: new Type, open NHamcrest.Core, yields in seq { yield ... }
    • cut down usings System.Collections.IEnumerable but System.Collections were opened
  • Follow style guidelines especially type declarations a: Type (docs.microsoft)
  • Old comments they were never touched

Altogether, the code is more readable.

@sergey-tihon
Copy link
Member

With such big reformatting, I believe it make sense to adopt https://github.com/fsprojects/fantomas
Slightly tune defaults to align with this project code style (more) and format code on each build from FAKE

@CaptnCodr do you want to try this?

@CaptnCodr
Copy link
Member Author

I didn't ever use Fantomas but I'll try this.

@sergey-tihon
Copy link
Member

Thank you! I also never integrate it with repo before.
Here you can find great talk from Florian with intro to Fantomas and live demo how to integrate it into new repo and configure to minimize PR diff (align with project styles) https://youtu.be/ybkYHYKYeNw?t=4467

@CaptnCodr
Copy link
Member Author

It does make sense to push the fantomas integration into this PR, isn't it?

@sergey-tihon
Copy link
Member

sergey-tihon commented Jun 30, 2020 via email

@CaptnCodr
Copy link
Member Author

Hey @sergey-tihon, I added Fantomas now. :)
I just use the Default config FormatConfig.Default plus only the code in src because there is ofCaseTests.fs which will be formatted on every build. I don't know why. I exclude the AssemblyInfo.fs because this file is autogenerated.

@sergey-tihon
Copy link
Member

I am not really have with formatting like
image

I've upgraded Fantomas to latest prerelease version and slightly upgrade config.
What do you think about current format?

@CaptnCodr
Copy link
Member Author

Oh yes. That's definitely better.

@CaptnCodr
Copy link
Member Author

We could replace all . in test names by [dot] and e.g. 1 .. 10 by 1 to 10 to have a better structural overview in VS.
Additionally, we could ship this PR with PR #147 in FsUnit v4.0? :)

# Conflicts:
#	paket.lock
#	src/FsUnit.NUnit/FsUnit.fs
#	src/FsUnit.NUnit/FsUnitTyped.fs
#	src/FsUnit.NUnit/GenericAssert.fs
#	tests/FsUnit.NUnit.Test/equalTests.fs
#	tests/FsUnit.Xunit.Test/equalTests.fs
@sergey-tihon sergey-tihon changed the title [Cosmetic] Refactorings [Cosmetic] Refactorings & Fantomas integration Jul 19, 2020
let f =
FastGenericEqualityComparer<'T>.Equals(x, y)

printfn "Ref returns %b" f
Copy link
Member

Choose a reason for hiding this comment

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

@marklam I missed this line in your formatting. Why do you printf to console here?

@sergey-tihon sergey-tihon mentioned this pull request Jul 19, 2020
14 tasks
@sergey-tihon sergey-tihon merged commit 7b02718 into fsprojects:master Jul 19, 2020
@CaptnCodr CaptnCodr deleted the Refactorings branch November 10, 2020 19:00
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.

2 participants