Skip to content
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

.NET Standard support #256

Open
nkreipke opened this issue Oct 6, 2016 · 4 comments
Open

.NET Standard support #256

nkreipke opened this issue Oct 6, 2016 · 4 comments

Comments

@nkreipke
Copy link
Contributor

nkreipke commented Oct 6, 2016

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
Copy link
Owner

mfenniak commented Oct 7, 2016

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Collaborator

bchavez commented Oct 14, 2016

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#.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants