Simplifying property configuration #346

dharmaturtle · 2021-09-11T05:03:32Z

Closes #343

Okay, I got sucked into this.

Marking as a draft cause I still need to

update tutorial/documentation
update changelog

Buuuuut for the most part I think it's mostly done.

…ithTests, and withConfig

dharmaturtle · 2021-09-11T05:04:38Z

src/Hedgehog/Linq/Property.fs

@@ -71,91 +71,46 @@ type PropertyExtensions private () =
        let (Property property) = property
        Property.report property



It might actually be nice to keep the config overloads. Thoughts?

I think we should only expose a single way to configure properties; this new API should be the only API.

I am not exactly sure what is being discussed here. However, I am not overly concerned with the C# API.

dharmaturtle · 2021-09-11T05:05:35Z

src/Hedgehog/Property.fs

-    let ofGen (x : Gen<Journal * Outcome<'a>>) : Property<'a> =
-        Property x
+    let ofGen (gen : Gen<Journal * Outcome<'a>>) : Property<'a> =
+        Property (PropertyConfig.defaultConfig, gen)


I gave the PropertyConfig.defaultConfig zero thought.

We'll need to audit the use of this function, if it's being used as prop |> toGen |> doGenThing |> ofGen, then we'll lose the original configuration.

Indeed. All such calls should be replaced with Property.Generator.map (as I structured/named it in another comment).

How about, it a PR before this one, we change Property from a single case discriminated union to a record with a single field as well as expressing all maps over that field as such? Then I think it would easier to verify where PropertyConfig.defaultConfig should be used.

dharmaturtle · 2021-09-11T05:07:55Z

src/Hedgehog/Property.fs

+        Property (config, gen)
+
+    /// Restores the default shrinking behavior.
+    let withoutShrinks (Property (config, gen) : Property<'a>) : Property<'a> =


I think this should be called withDefaultShrinks.

I'm okay with this, the current name withoutShrinks sounds as if no shrinking will occur.

I think a better name that withDefaultShrinks (which is a syntactic name) is withUnlimitedShrinking (which is a semantic name)

dharmaturtle · 2021-09-11T05:09:41Z

src/Hedgehog/Property.fs

-        |> Report.render
+    /// Set the number of times a property is allowed to shrink before the test
+    /// runner gives up and displays the counterexample.
+    let withShrinks (shrinkLimit : int<shrinks>) (Property (config, gen) : Property<'a>) : Property<'a> =


Might be nice to just take an int instead of an int<shrinks>

Can you expand on this?

TysonMN · 2021-09-11T12:31:03Z

src/Hedgehog/Property.fs

@@ -4,15 +4,15 @@ open System

 [<Struct>]
 type Property<'a> =
-    | Property of Gen<Journal * Outcome<'a>>
+    | Property of PropertyConfig * Gen<Journal * Outcome<'a>>


What about making this into a record? How about this?

{ Config: PropertyConfig Generator: Gen<Journal * Outcome<'a>> }

I'm okay with this, I think we should also mark the internals as private, and use lower camel case for the names:

// unsure of the validity of this syntax, just typing into the comment editor right now type Property<'a> = private { config: PropertyConfig generator: Gen<Journal * Outcome<'a>> }

In any case, we should make 2 accessor functions, and only use them. They can be marked as inline if we have performance concerns.

let inline config (property : Property<'a>) : PropertyConfig = property.config // This already exists as 'toGen' let inline generator (property : Property<'a>) : PropertyConfig = property.generator

Thoughts?

I am ok with removing the public access scope. However, I think the access scope should be internal instead of private.

I think we should follow the F# naming guidelines and use Pascal case for record field names.

The most common operation is mapping, not (just) getting. As such, I think we should create a submodule inside Property like this (@dharmaturtle knows what I mean):

module Config = let get p = p.Config let set p c = { p with Config = c } let map f p = { p with Config = f p.Config }

ghost

Let me just say, I love the work done here. So many extra methods/functions are now gone! 🚀

We'll need to look at this carefully, so the review may take a while but I have no doubt that this is the right way to go. Thank you very much for doing this @dharmaturtle!

ghost · 2021-09-11T17:25:27Z

tests/Hedgehog.Tests/ShrinkTests.fs

        let report =
            property {
                let! actual = Range.linear 1 1_000_000 |> Gen.int
                return actual < 500_000
-            } |> Property.reportWith propConfig
+            } |> Property.withShrinks shrinkLimit |> Property.report


These two |> should be on separate lines for readability.

ghost · 2021-09-11T17:27:14Z

src/Hedgehog/Linq/Property.fs

+    [<Extension>]
+    static member WithConfig (property : Property, config : PropertyConfig) : Property =
+        let (Property property) = property
+        Property (Property.withConfig config property)


I think we can get rid of this, and only expose configuration with Property.with*

This is the same as Property.Config.set (as I strucutured/named it in another comment). I think it is better to keep. Might want to consider renaming to setConfig though.

What would the value be in overriding a user's preferences?

It would be the user calling this function, it would be their preference. For example, @dharmaturtle talks about how he runs PB tests with a time limit during development and with a test limit during integration. I think the minimal amount of duplicated code to achieve this would be constructing the correct PropertyConfig instance in a global location and then referencing it from all the PB tests in order to set it, which (when using C#) would be achieved via this method.

You could also make an extension method of Property that configures them to all be the same and call that on each property you want it to apply to.

public static Property ConfigureSpecifically(this Property property) => property.WithTests(1000).WithoutShrinkLimit().Whatever(); // in a test var someProperty = // ... someProperty.ConfigureSpecifically().OverrideLocally().Check();

Good point.

I think I would still keep this myself, but I won't argue further in favor of keeping it.

ghost · 2021-09-11T17:28:41Z

src/Hedgehog/Linq/Property.fs

@@ -71,91 +71,46 @@ type PropertyExtensions private () =
        let (Property property) = property
        Property.report property



I think we should only expose a single way to configure properties; this new API should be the only API.

dharmaturtle · 2021-09-18T17:19:28Z

Gonna let #353 go first as it's smaller, more mature, and may possibly establish designs/patterns that this will follow.

Removed reportWith* and added Property.withShrinks, withoutShrinks, w…

08f4956

…ithTests, and withConfig

dharmaturtle commented Sep 11, 2021

View reviewed changes

TysonMN reviewed Sep 11, 2021

View reviewed changes

ghost suggested changes Sep 11, 2021

View reviewed changes

ghost added this to the 0.11.0 milestone Sep 11, 2021

ghost mentioned this pull request Sep 11, 2021

Generalized formatting of generated values #353

Closed

dharmaturtle marked this pull request as draft September 18, 2021 14:21

ghost modified the milestones: 0.11.0, 0.12.0 Sep 21, 2021

TysonMN mentioned this pull request Sep 23, 2021

Add more detailed state for property. #367

Closed

TysonMN force-pushed the master branch from d74c820 to 03c7125 Compare July 23, 2022 04:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplifying property configuration #346

Simplifying property configuration #346

dharmaturtle commented Sep 11, 2021

dharmaturtle Sep 11, 2021

ghost Sep 11, 2021

TysonMN Sep 11, 2021

dharmaturtle Sep 11, 2021

ghost Sep 11, 2021

TysonMN Sep 11, 2021

TysonMN Sep 12, 2021

dharmaturtle Sep 11, 2021

ghost Sep 11, 2021

TysonMN Sep 11, 2021

dharmaturtle Sep 11, 2021

ghost Sep 11, 2021

TysonMN Sep 11, 2021

ghost Sep 11, 2021 •

edited by ghost

Loading

TysonMN Sep 11, 2021

ghost left a comment

ghost Sep 11, 2021

ghost Sep 11, 2021

TysonMN Sep 11, 2021

ghost Sep 11, 2021

TysonMN Sep 12, 2021

ghost Sep 12, 2021

TysonMN Sep 12, 2021

ghost Sep 11, 2021

dharmaturtle commented Sep 18, 2021

		@@ -71,91 +71,46 @@ type PropertyExtensions private () =
		let (Property property) = property
		Property.report property

Simplifying property configuration #346

Are you sure you want to change the base?

Simplifying property configuration #346

Conversation

dharmaturtle commented Sep 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Sep 11, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dharmaturtle commented Sep 18, 2021

ghost Sep 11, 2021 •

edited by ghost

Loading