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

Upgraded to NH5. Support for async. #31

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gliljas
Copy link

@gliljas gliljas commented Oct 11, 2017

The upgraded nuspec has not been tested.
The sample web project didn't start here, but it's a bit dated.

@TheCloudlessSky
Copy link
Owner

Hi @gliljas,

Thanks for your work on this! I'm interested in merging this but there a few issues that would need to be addressed before I can do so:

  1. We must be able to support those that still depend on NHibernate < 5. Because NHibernate is such a fundamental library to applications, upgrading to NHibernate 5 isn't the easiest for everyone (I know this from personal experience). This means that the minimum version of which our Nuget package depends upon shouldn't have to change. I noticed in your nuspec changes that if targeting net461, use NHibernate >= 5.0. This makes sense from NHibernate 5 requiring .NET 4.6.1, but we need to make sure if someone is on .NET 4.6.1 that they can still use NHibernate < 5 (it's not immediately clear if this will work with Nuget). We need to make sure there is binary compatibility for NHibernate < 5. For example, if someone has version 3.0.0-beta5 of this library installed, would upgrading to the next version force NHibernate to also be upgraded (obviously if our Nuget dependency changed it would force it)?
  2. Some of the code doesn't match the existing code style (e.g. we only use spaces, extra whitespace lines, I don't like the idea of having a separate file for async, etc).
  3. There's a lot of superfluous changes to config files that aren't relevant to the NH 5 upgrade (e.g. what is AsyncGenerator.yml, why so many App.config changes, etc).

@gliljas
Copy link
Author

gliljas commented Oct 24, 2017

Thanks for replying. I will give it a bit of cleanup and an attempt to support NH<5 cleanly.

@gliljas
Copy link
Author

gliljas commented Oct 24, 2017

How would you like to handle the backwards compatibility? A separate Async package with >=5 dependency and changing the dependency of the standard package to <5?

@TheCloudlessSky
Copy link
Owner

Can we not do it in one package using binary compatibility?

@gliljas
Copy link
Author

gliljas commented Oct 24, 2017

I don't see how, or am I missing something? Or are you thinking of having a package version 5 (or 4) with a NH5 dependency, while still being able to maintain v3 packages for NH4?

@gliljas
Copy link
Author

gliljas commented Oct 26, 2017

The "standard" caches were just upgraded

https://www.nuget.org/packages/NHibernate.Caches.SysCache/5.1.0

..and they've chosen to just up the dependency. I suggest the same for this library.

@hazzik
Copy link

hazzik commented Oct 29, 2017

NH 5 isn't binary compatible with previous versions.

@fredericDelaporte
Copy link

People needing to use older NHibernate should just use the previous cache package version. NuGet allows this.

The drawback is for supplying new features to them along to the latest version: it will require to publish a new version of the old package along with a new version of the up-to-date package. As long as the up-to-date package has a major version greater than the packages targeting previous NHibernate, it is doable without confusion by just upgrading the minor part of the version of both.

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

Successfully merging this pull request may close these issues.

4 participants