CollaboratingPlatypus / PetaPoco

Official PetaPoco, A tiny ORM-ish thing for your POCO's
Other
2.06k stars 600 forks source link

Update PetaPoco to net standard #460

Closed pleb closed 5 years ago

pleb commented 6 years ago

It's time to update PetaPoco to net standard, and purpose of this ticket is to track the work.

This is a work in progress

A few questions I think we need to answer

Work required

asherber commented 6 years ago

I'm not entirely sure what the purpose is of assemblies that target different 4.x versions, since there don't appear to be any code differences. In the abstract, I would suggest targeting just 4.0 until such time as you add features that require a higher version. But since you have users who already have the other assemblies installed, you probably have to keep them around. Luckily, this is easy in a multi-targeted SDK project. I don't think there's any reason to add a 4.7 assembly.

In terms of .NET Standard, 2.0 is fairly easy to do. With lower versions, there are more things you need to adjust and/or bring in additional NuGet packages. I think you could make a case either way.

asherber commented 6 years ago

As a sidenote, there should be some discussion around how best to handle connection strings and other things that assume an app.config/web.config file. It's probably worth looking at how NPoco handles this.

asherber commented 6 years ago

Breaking change? I don't think it would break anything as long as the NuGet package is multi-targeted.

pleb commented 6 years ago

The reason for the assemblies targetting each .net version is that I found some people were confused and thought their target version is not supported, or PetaPoco is out of date.

Re the nuget packages, a fair few people think the Core in PetaPoco.Core and PetaPoco.Core.Complile means dotnet core. Maybe this won't be an issue if we can produce a .net standard assembly.

asherber commented 6 years ago

Looks like for .NET Standard, NPoco gets rid of any Database constructor that takes a provider name (because it uses DbProviderFactories to look up the factory, and that class is not available in Standard) and any constructor that takes a connection string name (because we can't count on app.config/web.config).

If we take the same approach here, I think we would just need to omit the constructor with a connection string name, because PetaPoco doesn't use DbProviderFactories, and maybe the no-param constructor, which makes some assumptions based on the presence of a config file. The fluent configuration would also need a little adjusting.

(For my PR, I included the System.Configuration.ConfigurationManager to get everything to "work", but it's likely that a Core project wouldn't actually have a config file like this.)

asherber commented 6 years ago

On naming: If I were naming libraries from scratch, I would probably have:

Note the different T4 packages because source and compiled assembly use different namespaces (#461)

pleb commented 6 years ago

I like the naming 👍

I wonder if we

asherber commented 6 years ago

I'm not sure that this should be two separate steps -- might just lead to more confusion. I get that adding .NET Standard first theoretically lets users do a drop-in replacement as they migrate their code, but considering that there will be changes to some of the constructors (because of changes to the configuration system), I don't know how common a use case that would be.

pleb commented 6 years ago

If we include a package dependency of System.Configuration.ConfigurationManager, we can avoid changes to the constructors

pleb commented 6 years ago

Ok, what about we target

standard2.0 for >=4.6.1 net46 for < 4.6.1 net45 for < 4.6.1 net40 for < 4.6.1

asherber commented 6 years ago

My impression is that the System.Configuration.ConfigurationManager package is only useful if you're using a Standard library in the context of a Framework project. If you're using the library in a .NET Core project, you're not going to have a legacy app.config/web.config file, so even though the methods would be available, they wouldn't serve any purpose. My vote would be for removing them from the Standard version and assuming that if you're using the Standard library, you're on your own in terms of providing configuration info to PetaPoco.

pleb commented 6 years ago

yeah, I can't see people using the old web.config/app.config in a core project. I'd like to try and get standard over the line without any breaking changes. I know we need breaking changes at some point, but I'd like to push these to version 2 of PP.

I'd like to quickly get standard support out, and then version 2 shortly after. I see version 2 including

  1. Updated configuration API surface to support FF and Core configuration subsystem nuances
  2. Single file with namespaces
  3. Renaming of nuget packages
  4. I'd love to drop the t4 template and replace it with .net core tooling
asherber commented 6 years ago

I guess I don't see dropping app.config support from the netstandard version as a breaking change, because it's a whole new version of the assembly -- that change won't affect anybody currently using PetaPoco (won't break any existing code).

I agree with the goals for v2 -- would like to hear more about what you have in mind for (1) and (4).

pleb commented 6 years ago
  1. Actually, maybe point one is a too hard basket. FF vs core. Sigh. It might be easier to only support connection string and not connection string by name. This way, a user is responsible for finding the connection string. This should allow for us to drop System.Configuration.ConfigurationManager dependancy

  2. Would be building the cli tooling https://github.com/aspnet/DotNetTools/. So I envisage something like

    dotnet petapoco set connection-string "sdfsdfsdf"
    dotnet petapoco set out-location "\Project\Data\Models"
    dotnet petapoco update
    dotnet petapoco update --table-filter *Users
asherber commented 6 years ago
  1. That's what I have in #463, specifically https://github.com/CollaboratingPlatypus/PetaPoco/pull/463/commits/05d44f13c2d93b588c9afa80aab537c682caf9f8 See if that works for you. If you're going to support net40, net45, and net46, then the conditionals will need some tweaking -- maybe define a NETFX property in the csproj that can be used to cover these cases.

  2. I'll have to learn more about that!

pleb commented 6 years ago

Looks good

asherber commented 6 years ago

I'm doing some poking around with the different providers in .NET Core. I haven't gotten to all of them, but MsAccess and System.Data.SQLite are not supported. Probably better not to include those providers in the Standard version, but maybe you should add an alternate SQLite provider based on Microsoft.Data.SQLite that could be available in both the Standard and Framework builds.

asherber commented 6 years ago

(With regard to packages and names, you could make the case that a package with the PetaPoco source file is no longer needed. When PP was first developed and was distributed as source, I think NuGet had not yet become omnipresent. Now that everyone uses it to manage [compiled] dependencies, I'm not sure what purpose a source distribution serves.)

pleb commented 6 years ago

OK, so I decided to roll version 6 and leave behind version 5. I did this because there are a few breaking changes, and I think we really only want to support one nuget package from this point forward.

Changes are in the v6 branch

Backports and updates to v5 will happen on the Master-V5 and Development-V5 branches.

I've started the wiki page, and plan to fill this out with more detail soon

The Net Standard 2.0 API surface does not include support for Full-Framework (FF) configuration.

The older T4 Templates will not have a nuget package. If people still require their use, they can download it from GH or clone the source.

Single file deployment has been retired, in favour of the dll lib version.

Integration tests are running for FF and .net core. There is a slight issue with the sqlite (net core) provider, but I'm not too worried about this, as the FF provider works as it did before.

Lastly, the nuget package name..... ideally, I'd like to simply call it PetaPoco, but that name is taken by the v5 single file. If we reuse this name, people will - no doubt - update and break their solutions. To avoid it, I think it's best to use a different name. I thought about PetaPoco.NetStandard, but we roll v4, v45, net 2.0 in there.

My thought regarding the package name:

picasso566 commented 6 years ago

Maybe OT for this thread but...

Is anyone working on converting T4 templates to another technology? I would like to help with that effort if it's not already in place or if anyone could use my help.

pleb commented 6 years ago

@picasso566 I was thinking that if the code gen feature is kept, it would be best as a dotnet tool (https://github.com/aspnet/DotNetTools/)

Before the current netcore sdk release, a dotnet tool was installed to the local project. However, now it can also be global. This reminds a little of NPM - if you've ever used it.

As for the API maybe, and this is just me thinking out loud....

dotnet petapoco generate --connection-string "sdfsdfsdf" --out-location "\Project\Data\Models" --filter-out "%User%" --filter "%Ordre%" --template "Default"|"Single file"

dotnet petapoco generate -cs "sdfsdfsdf" -ol "\Project\Data\Models" -fo "%User%" -f "%Ordre%" -t "Default"|"Single file"

Minimum

dotnet petapoco generate -cs "sdfsdfsdf" -ol "\Project\Data\Models"

after the first call is made, a shorter path to re-running it (would involve saving previous options)

dotnet petapoco update
asherber commented 6 years ago

I see your point about package naming; I agree that reusing the PetaPoco name could be problematic. I would vote for PetaPoco.Compiled, containing the Framework and Standard builds. The existing PetaPoco.Core.Compiled package could also become an umbrella package which simply contains PetaPoco.Compiled -- that would give an easy upgrade path to anyone using the existing package.

pleb commented 6 years ago

Beta is out

https://www.nuget.org/packages/PetaPoco.Compiled/

samchenws commented 6 years ago

I hope to petapoco join asynchronous.

pleb commented 5 years ago

@samchenws it's a WIP ;)