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

Obsoletes with warnings should be made into errors if their usage breaks code #3308

Open
bdrajer opened this issue May 24, 2023 · 1 comment

Comments

@bdrajer
Copy link

bdrajer commented May 24, 2023

I've upgraded from NHibernate 4.1 to 5.3 and soft obsoletes are starting to become a problem. For example, a line in my code like

if (obj is IProxy)

doesn't work anymore since IProxy is now unused - it doesn't detect proxies anymore. The IProxy interface has been marked as Obsolete but only produces a warning during build. It seems it would have been better if the interface were removed if it does nothing, or at least the Obsolete attribute set to produce a build error... Otherwise it becomes a trap.

The same goes for the QueueOperation() method in AbstractPersistentCollection, I've overridden it ages ago and haven't noticed it has become obsolete. As a result of the upgrade, my override doesn't get called and my code is broken. In such cases it would have been better for my build to break.

@hazzik
Copy link
Member

hazzik commented May 24, 2023

Thanks for submitting the issue. Unfortunately, we cannot travel back in time and change that postmortem. We'll try to communicate more clearly the obsolete behaviors in the future.

Upgrading from 4.1 to 5.3 is a long way and a lot of things has changed. It is always a good idea to review the release notes and enable "treat warnings as errors" at least temporarily while upgrade is in progress.

if (obj is IProxy) -- this was always invalid code working incidentally. IProxy was an internal-ish interface, a part of implementation details, a remain of copied LinFu.DynamicProxy. The correct check, from the dawn of times, was to check for implementation of INHibernateProxy, or calling NHibernateProxyHelper.IsProxy(obj) method. If you cannot change your code you should be able to switch back using DynamicProxyFactoryFactory.

As for AbstractPersistentCollection.QueueOperation I think we could have better clarified the changed behavior.

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

2 participants