Improved Fsharp integration #462

Closed
wants to merge 34 commits into
from

Projects

None yet

4 participants

@colinbull
Contributor

Hi,

I have added a library Raven.Client.Lightweight.FSharp.

Essentially it adds a set of combinators over the existing client API and a computation expression builder, mainly for syntactic sugar.

There are a couple of points,

  • A dependency on FSharp.Powerpack.Linq.Fixed exists because support for linq expressions is somewhat ropey in F#, this improves in F# 3.0, so it is only for < .NET 4
  • I have tried to bring the tests inline in the same way as the rest of the raven solution
  • I realise that I don't have 100% coverage of all the features, I have just gone with the stuff that I use most often. And due to the lack of support for Linq expressions , c# is still needed to define indexes.

Let me know what you think. I can be contacted directly on twitter via @colinbul

Cheers

Colin

@ravendb
ravendb commented Feb 25, 2012

Colun,
Do we have your CLA?

On Sat, Feb 25, 2012 at 8:20 PM, Colin <
reply@reply.github.com

wrote:

Hi,

I have added a library Raven.Client.Lightweight.FSharp.

Essentially it adds a set of combinators over the existing client API and
a computation expression builder, mainly for syntactic sugar.

There are a couple of points,

  • A dependency on FSharp.Powerpack.Linq.Fixed exists because support for
    linq expressions is somewhat ropey in F#, this improves in F# 3.0, so it is
    only for < .NET 4
  • I have tried to bring the tests inline in the same way as the rest of
    the raven solution
  • I realise that I don't have 100% coverage of all the features, I have
    just gone with the stuff that I use most often. And due to the lack of
    support for Linq expressions , c# is still needed to define indexes.

Let me know what you think. I can be contacted directly on twitter via
@colinbul

Cheers

Colin

You can merge this Pull Request by running:

git pull https://github.com/colinbull/ravendb master

Or you can view, comment on it, or merge it online at:

#462

-- Commit Summary --

  • Added a more idiomatic API for using raven from fsharp
  • Added a test for luceneQueries and query composition
  • Added a test for luceneQueries and query composition
  • moved tests inline for rest of Raven solution

-- File Changes --

A Raven.Client.Lightweight.FSharp/DocumentStore.fs (18)
A Raven.Client.Lightweight.FSharp/Helpers.fs (72)
A Raven.Client.Lightweight.FSharp/Raven.Client.Lightweight.FSharp.fsproj
(78)
A Raven.Client.Lightweight.FSharp/Raven.fs (153)
A Raven.Client.Lightweight.FSharp/Script.fsx (7)
A Raven.Client.Lightweight.FSharp/packages.config (5)
A Raven.Tests.FSharp/Raven.Tests.FSharp.fsproj (121)
A Raven.Tests.FSharp/RavenTests.fs (153)
A Raven.Tests.FSharp/TestData.fs (37)
A Raven.Tests.FSharp/packages.config (5)
M RavenDB.sln (24)
M Samples/Raven.Sample.FSharp/Program.fs (3)
M Samples/Raven.Sample.FSharp/Raven.Sample.FSharp.fsproj (5)
M Samples/Raven.Samples.sln (25)
A packages/FsLinqFixed.2.1.1/FsLinqFixed.2.1.1.nupkg (0)
A packages/FsLinqFixed.2.1.1/lib/net40/FSharp.PowerPack.Linq.Fixed.dll (0)
A packages/FsLinqFixed.2.1.1/lib/net40/FSharp.PowerPack.Linq.Fixed.pdb (0)
A packages/FsLinqFixed.2.1.1/lib/net40/FSharp.PowerPack.Linq.Fixed.xml
(367)
A packages/FsLinqFixed.2.1.1/tools/install.ps (10)
A packages/FsUnit.xUnit.1.0.0.4/Content/FsUnitSample.fs.pp (22)
A packages/FsUnit.xUnit.1.0.0.4/FsUnit.xUnit.1.0.0.4.nupkg (0)
A packages/FsUnit.xUnit.1.0.0.4/Lib/Net20/FsUnit.CustomMatchers.dll (0)
A packages/FsUnit.xUnit.1.0.0.4/Lib/Net20/FsUnit.Xunit.dll (0)
A packages/FsUnit.xUnit.1.0.0.4/Lib/Net20/FsUnit.Xunit.xml (78)
A packages/FsUnit.xUnit.1.0.0.4/Lib/Net20/NHamcrest.dll (0)
A packages/FsUnit.xUnit.1.0.0.4/Lib/Net40/FsUnit.CustomMatchers.dll (0)
A packages/FsUnit.xUnit.1.0.0.4/Lib/Net40/FsUnit.Xunit.dll (0)
A packages/FsUnit.xUnit.1.0.0.4/Lib/Net40/FsUnit.Xunit.xml (78)
A packages/FsUnit.xUnit.1.0.0.4/Lib/Net40/NHamcrest.dll (0)
M packages/repositories.config (2)
A packages/xunit.1.8.0.1549/lib/xunit.dll (0)
A packages/xunit.1.8.0.1549/lib/xunit.dll.tdnet (5)
A packages/xunit.1.8.0.1549/lib/xunit.runner.tdnet.dll (0)
A packages/xunit.1.8.0.1549/lib/xunit.runner.utility.dll (0)
A packages/xunit.1.8.0.1549/lib/xunit.xml (2446)
A packages/xunit.1.8.0.1549/xunit.1.8.0.1549.nupkg (0)

-- Patch Links --

https://github.com/ravendb/ravendb/pull/462.patch
https://github.com/ravendb/ravendb/pull/462.diff


Reply to this email directly or view it on GitHub:
#462

@colinbull
Contributor

Hi,

No, I will sign and send it to you tomorrow. Is there anything else you need?

Cheers

Colin.

@ravendb
ravendb commented Feb 26, 2012

Just the CLA, then I can pull the changes and review them

On Sun, Feb 26, 2012 at 3:29 AM, Colin <
reply@reply.github.com

wrote:

Hi,

No, I will sign and send it to you tomorrow. Is there anything else you
need?

Cheers

Colin.


Reply to this email directly or view it on GitHub:
#462 (comment)

@colinbull
Contributor

Send it to ayende email address

@ayende
Member
ayende commented Feb 27, 2012

storeImmediate, queryAllBy, queryAll, deleteImmediate, deleteMany, storeMany need to be removed.
They are violating some basic assumptions and best practices.

I also don't understand the Raven.docStoreMap
Why do you need to manage it in this fashion?

@colinbull
Contributor

Ok, no problem.. I thought this might be the case, for these functions, I
only bundled them because I tend to end up needing these in applications,
but I realise from an API point of view some of the assumptions they make
are bad, and application specific. Just out of curiosity what best
practices are being violated?

Now you mention it the docStore map is not necessary (I can't even remember
what I was thinking when I did this), I can just change the run function
to take an IDocumentSession instance. So it would then look something like
this.

use session = store.OpenSession()
raven {
do! store Foo
do! commit
} |> run session

How is that?

Cheers

Colin

On Mon, Feb 27, 2012 at 9:57 AM, Ayende Rahien <
reply@reply.github.com

wrote:

storeImmediate, queryAllBy, queryAll, deleteImmediate, deleteMany,
storeMany need to be removed.
They are violating some basic assumptions and best practices.

I also don't understand the Raven.docStoreMap
Why do you need to manage it in this fashion?


Reply to this email directly or view it on GitHub:
#462 (comment)

@ayende
Member
ayende commented Feb 27, 2012

The queryAll variant are bad because they assume that:

  • you can actually load the entire db to memory,
  • stable state with no changes
  • they result in N queries
  • they bypass the page size limits

the *Immediate variant are bad because they encourage immediate commit, vs
waiting for the code to complete.
In fact, I would say that you don't WANT to have a commit there, just have
it implicitly call SaveChanges() in the end if you didn't get an exception.

the *Many variants aren't needed, you can just do a foreach for them, so I
don't see why they are needed on the core API

I think that this API is better:

use session = store.OpenSession()
raven {
do! store Foo
} |> run session

By the mere fact that you completed the raven code without an exception,
SaveChanges() can be called.

On Mon, Feb 27, 2012 at 2:34 PM, Colin <
reply@reply.github.com

wrote:

Ok, no problem.. I thought this might be the case, for these functions, I
only bundled them because I tend to end up needing these in applications,
but I realise from an API point of view some of the assumptions they make
are bad, and application specific. Just out of curiosity what best
practices are being violated?

Now you mention it the docStore map is not necessary (I can't even remember
what I was thinking when I did this), I can just change the run function
to take an IDocumentSession instance. So it would then look something like
this.

use session = store.OpenSession()
raven {
do! store Foo
do! commit
} |> run session

How is that?

Cheers

Colin

On Mon, Feb 27, 2012 at 9:57 AM, Ayende Rahien <
reply@reply.github.com

wrote:

storeImmediate, queryAllBy, queryAll, deleteImmediate, deleteMany,
storeMany need to be removed.
They are violating some basic assumptions and best practices.

I also don't understand the Raven.docStoreMap
Why do you need to manage it in this fashion?


Reply to this email directly or view it on GitHub:
#462 (comment)


Reply to this email directly or view it on GitHub:
#462 (comment)

@colinbull
Contributor

Ahh ok thanks for that...

Ok, no problem, I'll make the changes and push tonight, as I don't have
access from where I am currently.

Cheers

Colin

On Mon, Feb 27, 2012 at 12:45 PM, Ayende Rahien <
reply@reply.github.com

wrote:

The queryAll variant are bad because they assume that:

  • you can actually load the entire db to memory,
  • stable state with no changes
  • they result in N queries
  • they bypass the page size limits

the *Immediate variant are bad because they encourage immediate commit, vs
waiting for the code to complete.
In fact, I would say that you don't WANT to have a commit there, just have
it implicitly call SaveChanges() in the end if you didn't get an exception.

the *Many variants aren't needed, you can just do a foreach for them, so I
don't see why they are needed on the core API

I think that this API is better:

use session = store.OpenSession()
raven {
do! store Foo
} |> run session

By the mere fact that you completed the raven code without an exception,
SaveChanges() can be called.

On Mon, Feb 27, 2012 at 2:34 PM, Colin <
reply@reply.github.com

wrote:

Ok, no problem.. I thought this might be the case, for these functions, I
only bundled them because I tend to end up needing these in applications,
but I realise from an API point of view some of the assumptions they make
are bad, and application specific. Just out of curiosity what best
practices are being violated?

Now you mention it the docStore map is not necessary (I can't even
remember
what I was thinking when I did this), I can just change the run function
to take an IDocumentSession instance. So it would then look something
like
this.

use session = store.OpenSession()
raven {
do! store Foo
do! commit
} |> run session

How is that?

Cheers

Colin

On Mon, Feb 27, 2012 at 9:57 AM, Ayende Rahien <
reply@reply.github.com

wrote:

storeImmediate, queryAllBy, queryAll, deleteImmediate, deleteMany,
storeMany need to be removed.
They are violating some basic assumptions and best practices.

I also don't understand the Raven.docStoreMap
Why do you need to manage it in this fashion?


Reply to this email directly or view it on GitHub:
#462 (comment)


Reply to this email directly or view it on GitHub:
#462 (comment)


Reply to this email directly or view it on GitHub:
#462 (comment)

@colinbull colinbull Added raven computation expression tests and a more complete expressi…
…on builder i.e. Now supports For, try/catch try/finally while ect. Also removed the combinators made bad assumptions under ayende's suggestion
7347173
@colinbull
Contributor

Ok, I have completed the changes we discussed earlier. I have also added more complete support and tests for the computational workflow syntax, this now supports things like

  • Try/Catch
  • Try/Finally
  • use and use!
  • Iteration like while and for

Cheers

Colin

@ayende
Member
ayende commented Feb 28, 2012

Why is there:

  • include
  • Is there a reason not to do implicit do! saveChanges at the end of the raven {} block?
@ayende
Member
ayende commented Feb 28, 2012

I meant include in quotes?

@colinbull
Contributor
@colinbull
Contributor

Oh, hold on, I think I see what you mean now.. so the

raven {} is like a discrete set of operations and if it all completes without exception then call save changes?

@ayende
Member
ayende commented Feb 28, 2012

Colin,
Yes, I think that this would result in better code.

As for include, how about we call it including, instead?

On Tue, Feb 28, 2012 at 12:24 PM, Colin <
reply@reply.github.com

wrote:

Oh, hold on, I think I see what you mean now.. so the

raven {} is like a discrete set of operations and if it all completes
without exception then call save changes?


Reply to this email directly or view it on GitHub:
#462 (comment)

@colinbull
Contributor

Yeah, including is good... That is actually what I had it as originally but I changed it to be more in line with the Raven API.

I'll get on to this later on tonight.

Cheers

@colinbull
Contributor

OK, have committed the changes you requested let me know if there is anything else

@ayende
Member
ayende commented Mar 2, 2012

We are almost there.
I pushed my changes to the fs branch in my fork:
https://github.com/ayende/ravendb/tree/fs

The current issue is: FsUnit.xUnit

It is using 1.8 xunit and we use 1.9

On Wed, Feb 29, 2012 at 8:25 PM, Colin <
reply@reply.github.com

wrote:

OK, have committed the changes you requested let me know if there is
anything else


Reply to this email directly or view it on GitHub:
#462 (comment)

@colinbull
Contributor

Hi,

I have just removed FSUnit and Updated the xUnit version to 1.9,

Experienced some oddities, thou with the upgrade for some reason the Assert.Equal was not selecting the correct overload, I guess this is something to do with the type inference. So in a few tests I have had to use Assert.True(expected = actual).

@ayende
Member
ayende commented Mar 4, 2012

Colin,
Thanks so much, I pulled your changes in.
Would you be able to write the docs for this as well?

@ayende ayende closed this Mar 4, 2012
@colinbull
Contributor

Not a problem..

Where do I include the docs?

@ayende
Member
ayende commented Mar 5, 2012

https://github.com/ravendb/docs is best

On Mon, Mar 5, 2012 at 11:06 AM, Colin <
reply@reply.github.com

wrote:

Not a problem..

Where do I include the docs?


Reply to this email directly or view it on GitHub:
#462 (comment)

@colinbull
Contributor

About the documentation,

F# projects do not support regions which seems to be how you are selecting the code samples in your documentation...

Will just embedding the code samples in the documentation suffice?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment