-
Notifications
You must be signed in to change notification settings - Fork 927
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
Remove WcfOperationSessionContext from .Net Core and .Net Standard #1842
Remove WcfOperationSessionContext from .Net Core and .Net Standard #1842
Conversation
c5d2c27
to
209c87d
Compare
/// .Net Framework distribution of NHibernate if you need it. See | ||
/// https://github.com/nhibernate/nhibernate-core/issues/1842 | ||
/// </summary> | ||
[Obsolete("Not supported in the .Net Core and .Net Standard distributions of NHibernate")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add “, true” probably
public WcfOperationSessionContext(ISessionFactoryImplementor factory) : base(factory) | ||
{ | ||
throw new NotSupportedException( | ||
"WcfOperationSessionContext is currently supported only by the .Net Framework distribution of NHibernate"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to change wording to emphasise that this is not supported in the current framework. The fact that it is only supported in full .NET Framework is not relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is, current framework can be .Net Framework, while using the .Net Standard binaries. In such case stating it is not supported by the current framework while the lack of support actually comes from the binaries would be confusing.
Granted, that is a corner case. Normally by using NuGet no-one should ends up using the .Net Standard binaries under .Net Framework, and currently NHibernate .Net Standard binaries are not provided anywhere else than on NuGet.
// consumes the .Net standard distribution of NHibernate instead of the .Net Framework one) | ||
// See https://github.com/dotnet/wcf/issues/1200 and #1842 | ||
throw new NotSupportedException( | ||
"WcfOperationSessionContext is currently supported only by the .Net Framework distribution of NHibernate"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as in the constructor.
{ | ||
public WcfOperationSessionContext(ISessionFactoryImplementor factory) : base(factory) | ||
{ | ||
throw new NotSupportedException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be PlatformNotSupportedException
// WCF server becames available in another frameworks or if a .Net Framework application | ||
// consumes the .Net standard distribution of NHibernate instead of the .Net Framework one) | ||
// See https://github.com/dotnet/wcf/issues/1200 and #1842 | ||
throw new NotSupportedException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be PlatformNotSupportedException
/// .Net Framework distribution of NHibernate if you need it. See | ||
/// https://github.com/nhibernate/nhibernate-core/issues/1842 | ||
/// </summary> | ||
[Obsolete("Not supported in the .Net Core and .Net Standard distributions of NHibernate", true)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it generic "Not supported in this platform"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with minor suggestions to the wordings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Currently, WCF Server support is a .Net Framework only feature.
On .Net Core, it does not seem it will be implemented any time soon, see dotnet/wcf#1200.
The
WcfOperationSessionContext
is meant to be used as a session context during server side calls, so providing it for .Net Core has currently no usages.Moreover, providing it for .Net Core or .Net Standard requires additional dependencies under .Net Core and .Net Standard, which creates some concerns (see #1820 & #1839).
The .Net Standard use case for it seems very tiny, it could be currently useful only if a .Net Framework WCF server application was using the .Net Standard distribution of NHibernate instead of the .Net Framework one. So better not provide it for .Net Standard too.
Replaces #1820 and #1840 (unless, for the later, if this PR cannot be merged in 5.2).