Implement C# Fluent API, along with LINQ #244

ghost · 2020-12-06T22:11:28Z

Fixes #172

I also followed the conversation on #183. I believe the best way to support C# is to just make a compatibility layer. I decided to target Gen initially to get some feedback before continuing. With that in mind, this PR is marked as a draft.

Pros

Overloads in the F# code looked ugly. ie. Defining a function called bind2 just to be able to apply the CompiledNameAttribute to support overloads. This is better accomplished with a class in F# since members allow overloading.
Func<_> incompatibilities. The current approach of adding "C# friendly" methods as needed would make this awkward to use from C#. Which module would a user import or access to get the method they're looking for?
All static methods. C# users would be more comfortable with a fluent API much like they get with LINQ now.
CSharp support in same namespace. I think it's easier to get a clean separation by introducing a namespace solely for C# support (Hedgehog.CSharp in this PR). Note that this isn't duplicating implementation of functions, just exposing a forwarding function for C# users.
Order of arguments is backwards from what C# users would expect. Another reason to introduce a compatibility layer is to "fix" the order of parameters so they're more natural to C# users.
Fable compatibility. Removing the CompiledNameAttribute from the core library helps Fable support because you can also remove the #if !FABLE_COMPILER ... #end directives. The directive can be thrown on all of the Hedgehog.CSharp types instead and not compile any of the compatibility layer when using Fable.
Type parameter names. A minor win, but we can control the names of type parameters so they are idiomatic for both C# (T, TResult) and F# ('a, 'b)

Cons

Having to duplicate documentation comments. Because the functions are separate from the methods, the documentation comments also separate, so they'd have to be duplicated.
Having to duplicate function declarations as methods.

Example usage:

using Hedgehog.CSharp;

static class Example
{
    public static void GenExample()
    {
        var date = Gen.DateTime // Gen<(TimeSpan, TimeSpan)[]>
                      .Where(dateTime => dateTime > DateTime.Parse("1900-01-01"))
                      .Select(dateTime => DateTime.Now - dateTime)
                      .Tuple()
                      .Array();
    }
}

moodmosaic

The idea looks good! Though I haven't pulled the branch locally yet. It'd be nice to have feedback from @Porges, @jwChung, and @mausch (ordered by date of opening/discussing the related issue(s)) about whether the overall direction matches what they have/had in mind.

src/Hedgehog/Gen.fs

dharmaturtle · 2020-12-15T15:44:44Z

Would it make sense to move the __ prefixed methods in this PR, as discussed in this issue, to these new files and namespace?

Retagging @mausch.

moodmosaic · 2020-12-15T15:51:13Z

Perhaps, yes, as that will reduce the 'noise'.

TysonMN · 2020-12-15T17:12:31Z

I would love it if those functions where defined in a separate file. I don't like looking at them when I am trying to think about and work with the core Hedgehog concepts.

ghost · 2020-12-16T05:50:29Z

@dharmaturtle, @moodmosaic, @TysonMN

I went ahead and finished porting the rest of the functions marked with [<CompiledName("...")>]. All of the ported functions are marked as extension methods where appropriate. This changed the way you check properties from Property.Check(prop) to prop.Check(). Not sure if there's a way to make extension methods that can be called both ways? Maybe something magical going on with the inline keyword on the extensions?

Either way, the rest of the modules could be ported as well, but they didn't seem to have pre-established C# support so I left them out.

moodmosaic · 2020-12-16T05:57:26Z

Looking forward trying this! Thank you for all the hard work 🍻

ghost · 2021-01-05T20:59:27Z

I think this should be evaluated again now that 0.9.0 is released. Having a better C# story would help the library quite a bit in the adoption department.

@moodmosaic @TysonMN

moodmosaic · 2021-01-05T21:28:12Z

Agreed.

ghost · 2021-01-05T22:54:03Z

I've rebased on master, everything is building and working. I've tried a few handful of permutations of the new API, all appears to be well. Unless we want all of the modules in the F# side to be exposed, I think this is ready to merge.

Edit: Not sure what happened in that last build, says certain packages don't exist in nuget?

TysonMN

Overall, looks great.

What is visible in C# now? For example, Are Gen.Digit and Gen.digit both visible?

Maybe in this PR or a future one, the output of Property.Check can give the C# code to recheck a failed property.

src/Hedgehog/Property.fs

tests/Hedgehog.CSharp.Tests/NameTests.cs

TysonMN · 2021-01-06T01:52:13Z

What is visible in C# now? For example, Are Gen.Digit and Gen.digit both visible?

Oh, maybe you already answered this when you said

Unless we want all of the modules in the F# side to be exposed...

How is it that the F# modules are not exposed?

ghost · 2021-01-06T01:58:54Z

What is visible in C# now? For example, Are Gen.Digit and Gen.digit both visible?

Oh, maybe you already answered this when you said

Unless we want all of the modules in the F# side to be exposed...

How is it that the F# modules are not exposed?

I mean that they are exposed in the C# API. Both would be visible to C# users, with the advisory that they use the Hedgehog.CSharp namespace.

TysonMN · 2021-01-06T02:13:04Z

I think it might be better to not expose the F# modules, but I don't think this PR needs to wait for that change.

...with the advisory that they use the Hedgehog.CSharp namespace.

Assuming we don't expose the F# modules, would it be possible to expose the C# code in the same namespace, namely Hedgehog?

ghost · 2021-01-06T02:22:05Z

Assuming we don't expose the F# modules, would it be possible to expose the C# code in the same namespace, namely Hedgehog?

I think the only way to do that would be to provide a separate nuget package for C# users, and use some directives to change the functions or modules to be private? I don't think it's possible to just hide F# code from other .NET languages in the same package meant for F# consumption.

moodmosaic · 2021-01-06T16:29:22Z

Maybe in this PR or a future one, the output of Property.Check can give the C# code to recheck a failed property.

That would be a great addition. 👍

would be to provide a separate nuget package for C# user

I'm wondering how other projects deal with this 🤔

TysonMN · 2021-01-06T20:53:31Z

@adam-becker, there is a conflict in Gen.fs.

ghost · 2021-01-06T21:52:59Z

@TysonMN I saw that, I'm still at work and haven't had a chance to get to it yet.

moodmosaic

Massive piece of work 🎢 💯 🚀 🦄 👍

This makes the F# part a lot cleaner as there are no attributes related to C# and no preprocessor directives related to Fable.

(I wonder if we could use T4 to generate all the stuff in CSharp/Xyz.fs.)

@adam-becker, if you can rebase this with master (there's a conflict around Gen.dateTime) I believe we can consider merging this. 🍻

src/Hedgehog/Property.fs

src/Hedgehog/Range.fs

tests/Hedgehog.CSharp.Tests/NameTests.cs

ghost · 2021-01-08T16:45:07Z

@moodmosaic Concerning the CompiledNameAttribute, I think we can add them back in for now but we should probably investigate the issues that @TysonMN raised with blocking any F# functions from being used from C#. That work could also address your concerns.

moodmosaic · 2021-01-08T16:55:19Z

@moodmosaic Concerning the CompiledNameAttribute, I think we can add them back in for now but we should probably investigate the issues that @TysonMN raised with blocking any F# functions from being used from C#. That work could also address your concerns.

We could add back in only those I mentioned in the comments above.

ghost · 2021-01-08T17:03:00Z

We could add back in only those I mentioned in the comments above.

Is it only those? I see a few more functions in src/Hedgehog/Range.fs that are inline or the return type wouldn't be inferrable in C#. scaleLinear, scaleExponential and constantBounded are the ones I've noticed, basically anything that has a type ^a I don't think would play nice in C#.

Also, I think this idea might be viable:

#if CSHARP
module internal Gen =
#else
module Gen =
#endif

We'd have to do 2 separate builds one with that symbol defined, one without. But this would hide all of the F# API for the C# package. We could also do the opposite for hiding the C# API from F#.

dharmaturtle · 2021-01-09T00:16:21Z

You can use the CompilerMessage attribute to hide code from F#. It has a property called IsHidden, and the docs say it

Indicates if the construct should always be hidden in an editing environment.

However, it only hides from F# editing environments. It still displays in C#... at least in Rider and Visual Studio. Dunno how to check what Ionide does.

E.g. this module....

module Asdf =
    [<CompiledName("HerpDerp"); CompilerMessage("This method is not intended for use from F#.", 10001, IsHidden=true, IsError=false)>]
    let Qwerty = ()

looks like this in C# VS:

but looks like this in F# VS:

To be clear, you can still call it; it just won't show up in Intellisense:

C# Rider:

F# Rider:

moodmosaic · 2021-01-10T09:23:21Z

Yep, it's tricky to actually hide those... We've tried also this in the past: #156 (comment)

moodmosaic

@TysonMN's suggestion to tackle #156 separately makes sense, given the size and context of this pull request.

However, since now it's going to be explicit whether you're using Hedgehog vs Hedgehog.CSharp, perhaps there's no need to think about #156.

The idea could be that

if you're using Hedgehog.CSharp from C# code, nothing you pick up via IntelliSense should throw a NotSupportException
if you're using stuff from Hedgehog (not Hedgehog.CSharp) from C# code, then anything that uses a statically resolved type parameter could throw...

Ready to merge at this point, once those 2 small suggestions I just left are resolved.

src/Hedgehog/CSharp/Property.fs

tests/Hedgehog.CSharp.Tests/LinqTests.cs

ghost · 2021-01-10T19:32:28Z

@moodmosaic Ready to merge!

moodmosaic · 2021-01-10T20:50:55Z

Great! Sometimes we can merge via a merge commit, and sometimes via squash and merge. What would you prefer?

ghost · 2021-01-10T20:55:18Z

Great! Sometimes we can merge via a merge commit, and sometimes via squash and merge. What would you prefer?

Either is fine with me

moodmosaic · 2021-01-10T21:27:47Z

src/Hedgehog/CSharp/Property.fs


    static member FromBool (value : bool) : Property<unit> =
        Property.ofBool(value)

    static member FromGen (gen : Gen<Journal * Result<'T>>) : Property<'T> =
        Property.ofGen gen

+    static member FromResult (result : Result<'T>) : Property<'T> =
+        Property.ofResult result
+


Yeah, if we could generate all src/Hedgehog/CSharp/*.fs files via T4 it'd be awesome (T4 is a pain to work with OTOH). 👍

moodmosaic · 2021-01-10T21:44:42Z

What a milestone 🚀 🌕 Thank you @adam-becker, and also @TysonMN and @dharmaturtle for the feedback. 👍

moodmosaic reviewed Dec 6, 2020

View reviewed changes

src/Hedgehog/Gen.fs Outdated Show resolved Hide resolved

moodmosaic marked this pull request as ready for review December 16, 2020 05:53

TysonMN approved these changes Jan 6, 2021

View reviewed changes

src/Hedgehog/Property.fs Show resolved Hide resolved

tests/Hedgehog.CSharp.Tests/NameTests.cs Show resolved Hide resolved

moodmosaic reviewed Jan 8, 2021

View reviewed changes

Adam Becker added 11 commits January 8, 2021 16:44

Implement C# Fluent API, along with LINQ

33f9c28

Might help to write the extensions correctly

e48a6af

Remove vestigial code from a previous experiment.

efb7eb8

Add type annotations

f558a76

Complete layer for Range, Property, and Gen

9a1907c

Missed a function.

78e0468

Fix minor indentation issue

7070f31

Consistency changes.

2c99952

Straightening out float types.

093ab5e

Use member instead of member inline.

a6db498

Make the extension classes static.

97f248c

Adam Becker added 2 commits January 8, 2021 16:48

fromThrowing -> ofThrowing

33e2c33

Fix C# API with datetime changes

821a91c

Adam Becker added 2 commits January 10, 2021 01:47

More idiomatic for C#

70e22d5

Better use of overloads

02df74a

moodmosaic reviewed Jan 10, 2021

View reviewed changes

src/Hedgehog/CSharp/Property.fs Outdated Show resolved Hide resolved

src/Hedgehog/CSharp/Property.fs Outdated Show resolved Hide resolved

tests/Hedgehog.CSharp.Tests/LinqTests.cs Outdated Show resolved Hide resolved

Adam Becker added 2 commits January 10, 2021 19:27

Remove accidental direct recursion

91cd8db

Unnecessary parens

e6f0e7c

moodmosaic reviewed Jan 10, 2021

View reviewed changes

moodmosaic merged commit 9fd37cd into hedgehogqa:master Jan 10, 2021

ghost deleted the csharp branch January 10, 2021 21:48

ghost mentioned this pull request Jan 20, 2021

choiceRec is not really usable from C# #171

Closed

moodmosaic mentioned this pull request Jan 30, 2021

Release 0.10.0 #280

Closed

ghost added this to the 0.10.0 milestone Jan 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement C# Fluent API, along with LINQ #244

Implement C# Fluent API, along with LINQ #244

ghost commented Dec 6, 2020

moodmosaic left a comment •

edited

Loading

dharmaturtle commented Dec 15, 2020

moodmosaic commented Dec 15, 2020

TysonMN commented Dec 15, 2020

ghost commented Dec 16, 2020

moodmosaic commented Dec 16, 2020

ghost commented Jan 5, 2021 •

edited by ghost

Loading

moodmosaic commented Jan 5, 2021

ghost commented Jan 5, 2021 •

edited by ghost

Loading

TysonMN left a comment

TysonMN commented Jan 6, 2021

ghost commented Jan 6, 2021

TysonMN commented Jan 6, 2021

ghost commented Jan 6, 2021

moodmosaic commented Jan 6, 2021

TysonMN commented Jan 6, 2021

ghost commented Jan 6, 2021

moodmosaic left a comment

ghost commented Jan 8, 2021

moodmosaic commented Jan 8, 2021

ghost commented Jan 8, 2021 •

edited by ghost

Loading

dharmaturtle commented Jan 9, 2021

moodmosaic commented Jan 10, 2021

moodmosaic left a comment •

edited

Loading

ghost commented Jan 10, 2021

moodmosaic commented Jan 10, 2021

ghost commented Jan 10, 2021

moodmosaic Jan 10, 2021

moodmosaic commented Jan 10, 2021

Implement C# Fluent API, along with LINQ #244

Implement C# Fluent API, along with LINQ #244

Conversation

ghost commented Dec 6, 2020

Pros

Cons

moodmosaic left a comment • edited Loading

Choose a reason for hiding this comment

dharmaturtle commented Dec 15, 2020

moodmosaic commented Dec 15, 2020

TysonMN commented Dec 15, 2020

ghost commented Dec 16, 2020

moodmosaic commented Dec 16, 2020

ghost commented Jan 5, 2021 • edited by ghost Loading

moodmosaic commented Jan 5, 2021

ghost commented Jan 5, 2021 • edited by ghost Loading

TysonMN left a comment

Choose a reason for hiding this comment

TysonMN commented Jan 6, 2021

ghost commented Jan 6, 2021

TysonMN commented Jan 6, 2021

ghost commented Jan 6, 2021

moodmosaic commented Jan 6, 2021

TysonMN commented Jan 6, 2021

ghost commented Jan 6, 2021

moodmosaic left a comment

Choose a reason for hiding this comment

ghost commented Jan 8, 2021

moodmosaic commented Jan 8, 2021

ghost commented Jan 8, 2021 • edited by ghost Loading

dharmaturtle commented Jan 9, 2021

moodmosaic commented Jan 10, 2021

moodmosaic left a comment • edited Loading

Choose a reason for hiding this comment

ghost commented Jan 10, 2021

moodmosaic commented Jan 10, 2021

ghost commented Jan 10, 2021

moodmosaic Jan 10, 2021

Choose a reason for hiding this comment

moodmosaic commented Jan 10, 2021

moodmosaic left a comment •

edited

Loading

ghost commented Jan 5, 2021 •

edited by ghost

Loading

ghost commented Jan 5, 2021 •

edited by ghost

Loading

ghost commented Jan 8, 2021 •

edited by ghost

Loading

moodmosaic left a comment •

edited

Loading