feat: Add motivation and examples for classical assertions #144
Conversation
| @@ -16,11 +16,29 @@ Consider using the constraint model, Assert.That(expr, Is.False), instead of the | |||
|
|
|||
| ## Motivation | |||
|
|
|||
| ADD MOTIVATION HERE | |||
| The classic Assert model contains less flexibility and reads more clumsy than the constraint model, | |||
There was a problem hiding this comment.
I would not agree that Assert.False(expression) is clumsy compared to Assert.That(expression, Is.False), the opposite to me. Maybe change to something about being consistent with which API to use in a test suite?
Also this rule is default disabled right? I've been thinking about adding rules + fixes for refactoring in the other direction. Been doing it manually a couple of times.
There was a problem hiding this comment.
I agree, 'clumsy' isn't maybe the word. We do consider them legacy APIs though.
Assert.False actually is a call to Assert.That(actual, Is.False) and I personally don't mind it. I do personally dislike AreEqual and on top of that I continually see people mix up the expected and actual parameters.
There was a problem hiding this comment.
Are there plans on removing them in a future version? Probably good to check usage statistics before doing it.
There was a problem hiding this comment.
I don't believe there are any plans to remove them.
There was a problem hiding this comment.
I would not agree that Assert.False(expression) is clumsy compared to Assert.That(expression, Is.False), the opposite to me. Maybe change to something about being consistent with which API to use in a test suite?
"Clumsy" was my poor attempt at writing that often Assert.True(expression) ends up reading like Yoda expressions. I'll try to see if I can come up with a better description - and I'm open for suggestions.
Also this rule is default disabled right? I've been thinking about adding rules + fixes for refactoring in the other direction. Been doing it manually a couple of times.
It is a warning, but it is enabled as default (currently none of the analyzers are disabled as default). Perhaps we should have analyzers, code fixes in both directions and then people can select the one that matches their setup (but I'm unsure if people would ever turn on an analyzer, or it just will be wasted work).
Regarding removing the classical model, then I think that originally (around 3.0 or before) there were plans to remove the classical model. This is no longer the case, but most work is done on the constraint model now.
There was a problem hiding this comment.
Agreed default disabled making it opt-in has somewhat poor discoverability.
We can maybe use the github API to figure out if classical model is more widely used? It is rare to find usage of Assert.that in my experience. While opinionated analyzers are nice for consistency it is unfortunate to push people away from their preferred API if there are no strong reasons for it.
An option is to make it default disabled to start with and change it later if we find solid data.
but I'm unsure if people would ever turn on an analyzer, or it just will be wasted work
I would turn it on :)
If you create issues for the other direction you can assign me. Should be pretty fast to write analyzers and fixes for the common cases.
There was a problem hiding this comment.
Maybe a new issue is better as this is already merged. I'll create one.
And correct title, message, and description of NUnit2005 and NUnit2006.