-
Notifications
You must be signed in to change notification settings - Fork 30
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
Split Property.fs across multiple files #247
Conversation
I love your intensity @adam-becker! Keep on proposing changes like this that you think are an improvement :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great 👍 If @TysonMN also approves, we can merge it. Shall we wait for v0.9.0 first, or merge it and then do v0.9.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. I highly prefer the new organization. I think this can be merged as is.
I left optional comments. I think all of them are independent of this reorganization.
With all of the suggestions from @TysonMN it might make sense to apply all of them and release 0.9.0 as a breaking version. Although anything before 1.0.0 is free to break things according to SemVer guidelines. That said, this library does have users and we should probably stuff as many breaking changes into a single version bump, instead of breaking constantly. @moodmosaic Do we want to make a project for the 0.9.0 release and quantify some breaking work we want to do? |
This is excellent work! I think we should cut a release first what's in |
I just reviewed again. Looks good to me. |
Should be ready to merge after resolving the conflicts. I've also left a small comment in #247 (comment). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready to merge after resolving those conflicts. Massive piece of work, @adam-becker, fantastic feedback, @TysonMN.
The test failed randomly, due to exhausting its discards. I'll make an issue to fix flaky tests. |
One last thing, before we merge, if you open Script.fsx you'll notice that it'll error about some missing references. All we have to do is just add them: @@ -1,13 +1,18 @@
#if INTERACTIVE
-#load "Numeric.fs"
+#load "AutoOpen.fs"
+ "Numeric.fs"
"Seed.fs"
"Tree.fs"
"Range.fs"
"Random.fs"
"Shrink.fs"
"Gen.fs"
+ "Journal.fs"
+ "Tuple.fs"
+ "Outcome.fs"
+ "Report.fs"
"Property.fs"
#endif
open Hedgehog
open System I can make that change myself of course, if you don't have time to do it. |
@moodmosaic Ready to go on this one. |
🎉 |
Fixes #246.
Also, while I moved
Result<'a>
I opted to rename it toOutcome<'a>
, therefor this also fixes #181.