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

Making Constraint.Description abstract breaks backwards compatibility #4610

Closed
bodewig opened this issue Jan 27, 2024 · 5 comments
Closed

Making Constraint.Description abstract breaks backwards compatibility #4610

bodewig opened this issue Jan 27, 2024 · 5 comments

Comments

@bodewig
Copy link

bodewig commented Jan 27, 2024

Hi,

over at XMLUnit.NET we've received a report our Constraints would no longer work with NUnit 4.x (see xmlunit/xmlunit.net#40 ).

The Constraint class' Desciption used to be virtual with a protected setter and XMLUnit's constraints used that setter instead of overriding the property. The property has now been changed into an abstract property with bf5d0dc nine months ago. Of course I can override the property in XMLUnit's constraints and it should work with 3.x and 4.x, but before I do so I wanted to ask whether this change has really been made intentionally. If so; I'd ask you to list it as a breaking change, I wouldn't be surprised if XMLUnit was not the only library providing IConstraint implementations that is affected by this.

@OsirisTerje
Copy link
Member

OsirisTerje commented Jan 27, 2024

It was done as part of a huge effort to make the library honor nullability, and also clean up some practices. The property should be read-only with the implemented class defining the value. I believe that was the reason. @manfred-brands , can you add to this ?

And sure, it should be listed.
Actually we might have some more of these, so thanks for alerting us on this one!

@manfred-brands
Copy link
Member

@OsirisTerje Yes this was done for nullability changes. The previous design relied on a derived class calling the protected setter which could not be guaranteed. It was either marking the property as nullable and add lots of tests on null or make it virtual and non-nullable. . As it is used in places like: Left.Description + " and " + Right.Description I choose for the latter.

For the same reason a change was made in the PrefixConstraint. Instead of relying on a derived class setting the DescriptionPrefix this now must be passed in to the PrefixContraint constructor.

@OsirisTerje
Copy link
Member

Thanks @manfred-brands ! That added some more context to it.

@bodewig We'll add it to the release notes :-)

@bodewig
Copy link
Author

bodewig commented Jan 28, 2024

I completely understand the rationale. Fixing stuff that feels wrong is what major versions are there for. I'll try to ensure the xmlunit-nunit3-constraints library is going to work with NUnit 4.x as well and create a nunit4 one if that is necessary. Right now that doesn't seem to be the case as the libraries really only provide new Constraints..

Thank you!

@OsirisTerje
Copy link
Member

Fixing with PR nunit/docs#916

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants