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

Remove backward pipe #276

Merged
merged 5 commits into from Jan 20, 2021
Merged

Remove backward pipe #276

merged 5 commits into from Jan 20, 2021

Conversation

ghost
Copy link

@ghost ghost commented Jan 18, 2021

As part of a discussion on #274 there was a question about removing usages of the <| operator. This PR demonstrates what that would look like, and is also merge-able.

/cc @moodmosaic @TysonMN

@ghost ghost mentioned this pull request Jan 18, 2021
doc/tutorial.md Outdated Show resolved Hide resolved
Copy link
Member

@moodmosaic moodmosaic 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. We could merge after you're done with @TysonMN's suggestion (or when you and Tyson are ready). 💯

src/Hedgehog/Gen.fs Outdated Show resolved Hide resolved
src/Hedgehog/Gen.fs Show resolved Hide resolved
src/Hedgehog/Gen.fs Show resolved Hide resolved
src/Hedgehog/Gen.fs Show resolved Hide resolved
src/Hedgehog/Gen.fs Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Jan 18, 2021

I think this is ready for another round of review.

Comment on lines -254 to +257
Property.print <| property {
Property.print (property {
let! xs = Gen.list (Range.linear 0 100) version
return xs |> List.rev = xs
}
})
Copy link
Member

Choose a reason for hiding this comment

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

I would do either

property {
} |> Property.print

or

property {
}
|> Property.print

but this is fine too.

There are other cases like this. I won't comment on them until we decide what is preferred here.

Copy link
Author

Choose a reason for hiding this comment

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

I think I would prefer

let name =
    property {
        // ..
    }
Property.print name

Often times F# code plays chicken with the off-side rule and I'd rather just be blunt and direct.

This is actually one situation where I think <| makes perfect sense to use. Passing a CE directly to a function as the last (or only) parameter, when no other piping is being used.

Property.print <| property {
    // ...
}

Copy link
Member

Choose a reason for hiding this comment

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

I am fine with anything here

doc/tutorial.md Outdated Show resolved Hide resolved
doc/tutorial.md Outdated Show resolved Hide resolved
doc/tutorial.md Outdated Show resolved Hide resolved
src/Hedgehog/Gen.fs Outdated Show resolved Hide resolved
src/Hedgehog/Gen.fs Outdated Show resolved Hide resolved
src/Hedgehog/Gen.fs Outdated Show resolved Hide resolved
src/Hedgehog/Gen.fs Outdated Show resolved Hide resolved
src/Hedgehog/Gen.fs Outdated Show resolved Hide resolved
tests/Hedgehog.Tests/MinimalTests.fs Outdated Show resolved Hide resolved
src/Hedgehog/Range.fs Outdated Show resolved Hide resolved
Co-authored-by: Nikos Baxevanis <nikos.baxevanis@gmail.com>
@ghost
Copy link
Author

ghost commented Jan 19, 2021

Pipeline failed because of some random "Unable to download dependencies" issue, is there a way to re-run the build?

@moodmosaic
Copy link
Member

I just rerun the pipeline. 👍

@moodmosaic
Copy link
Member

Alrighty, ready to merge?

@TysonMN
Copy link
Member

TysonMN commented Jan 20, 2021

Yep

@moodmosaic
Copy link
Member

🚀

@moodmosaic moodmosaic merged commit 42311bb into hedgehogqa:master Jan 20, 2021
@ghost ghost deleted the remove-backward-pipe branch January 20, 2021 16:10
@ghost ghost added this to the 0.10.0 milestone Jan 31, 2021
@ghost ghost mentioned this pull request Feb 3, 2021
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