-
Notifications
You must be signed in to change notification settings - Fork 37
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
Comments
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. :-) |
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 :-) |
The builder approach sounds great to me. I like the implementation of 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. |
FWIW, you can also use this |
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?
The text was updated successfully, but these errors were encountered: