mfenniak / rethinkdb-net

A C# / .NET client driver for RethinkDB.
Other
247 stars 37 forks source link

.NET Standard support #256

Open nkreipke opened 7 years ago

nkreipke commented 7 years ago

Hi @mfenniak!

I've been working on porting rethinkdb-net to .NET Standard: https://github.com/nkreipke/rethinkdb-net/tree/netstandard

Currently, rethinkdb-net and rethinkdb-net-test are both fully ported and pass all tests. I've also changed the configuration format to JSON as System.Configuration is unavailable in .NET Standard (we could conditionally compile this so .NET 4.5 users keep using their App.config) and updated the CircleCI scripts to use .NET Core for testing.

I'm not quite done yet (.NET 4.5 support is still missing), but I wanted to get some feedback before I submit a pull request. What do you think?

mfenniak commented 7 years ago

Hey @nkreipke.

I think this sounds like a great idea. I'm not too familiar with all the new .NET API levels, but it sounds like it would be really valuable for non-standard platforms to be able to access RethinkDB.

I would suggest that where possible, it's probably better to use supplemental assemblies rather than conditional compilation. In my experience, conditional compilation is annoying to maintain over time. For example, it'd be preferable to have a .NET 4.5 assembly that provides the System.Configuration capabilities (eg. rethinkdb-net-netconfig?), rather than having it conditionally compiled in one central assembly.

When it's ready, and it has at least a good upgrade path for current users, I'd be happy to review and merge a PR. Let me know how else I can help out; I don't have a lot of spare time, but I'd sure like to do anything I can to encourage a new contributor. :-)

nkreipke commented 7 years ago

I've now moved the System.Configuration part to a separate rethinkdb-net-appconfig project and replaced the ConfigurationAssembler class with a more flexible ConnectionFactoryBuilder. You could now create a connection factory like this:

var factory = new ConnectionFactoryBuilder().FromAppConfiguration().Build("testCluster");

One issue I found with a separate appconfig assembly is that users will have to adjust the section configuration in addition to installing a new package:

<section name="rethinkdb" type="RethinkDb.AppConfig.RethinkDbClientSection, RethinkDb.AppConfig"/>

I've left a deprecation notice in ConfigurationAssembler that notifies users of the changes, however requiring users to change their configuration might cause inconvenience. We might be able to blame Microsoft and their ridiculous System.Configuration architecture for this ;-)

Apart from that, I removed the JSON configuration as we already provide an interface to Microsoft.Extensions.Configuration, which allows loading all sorts of configuration formats. Reading a JSON file would be relatively easy (probably even easier in ASP.NET Core as they're using DI for this):

var config = new ConfigurationBuilder().AddJsonFile("rethinkdb.json").Build();
var factory = new ConnectionFactoryBuilder().FromConfiguration(config).Build("testCluster");

You could also build a connection factory directly with a ClusterElement class.

While porting RethinkDb.Newtonsoft I found some duplicated code where the connection factory is created, but the builder class solves the problem quite elegantly:

var factory = new ConnectionFactoryBuilder()
    .FromAppConfiguration()
    .UseNewtonsoftJsonSerializer()
    .Build("testCluster");

I think all that's left to do for now is updating the examples and documentation. Just wanted to ask if you're OK with these changes :-)

mfenniak commented 7 years ago

The builder approach sounds great to me. I like the implementation of UseNewtonsoftJsonSerializer as an extension method on the builder; that approach seems really easy to use while still being very flexible to additional extensions in the future.

A little unfortunate that the config file namespace changes, but, I think that's pretty easy to document and not a very onerous change for people to make.

bchavez commented 7 years ago

FWIW, you can also use this bchavez/C# driver with RethinkDB which already supports .NET Core and net45. However, bchavez/C# driver follows more closely the design of the official drivers & official documentation meaning you'll need to use ReQL in C#.