Add "to have properties satisfying" and deprecate value in "to have property"#687
Add "to have properties satisfying" and deprecate value in "to have property"#687alexjeffburke wants to merge 4 commits intomasterfrom
Conversation
|
If ignoring keys with undefined values is itself a little too special, I can return this to behaving like “to have keys satisfying” - but either way I think we need this so we can proceed. |
|
This feature need to go hand in hand with deprecating @papandreou how to you feel about ignoring the undefined properties? It seems in line with other assertions, but is it surprising? You could argue that expect({}, 'to satisfy', { foo: undefined }); |
|
@sunesimonsen thanks for reminding me about the optional value form - agreed that it needs deprecation alongside this. Will add my take on a warning and your test suggestions :) |
Clarify the different cases of passing an assertion argument and an expect.it argument. Make the expect.it tests representative i.e. use examples that would not be implemented with normal assertions.
Arrange for this to contain the logic for showing once. Do a little extra work to make sure the warning is coloured on the console.
|
@sunesimonsen I've gone through the tests and clarified the assertion cases from those requiring an |
|
Explicitly |
papandreou
left a comment
There was a problem hiding this comment.
I’m not sure I like “to have properties satisfying” as an assertion name. It’s testing property names and not the value, so I don’t intuitively understand what it does from the name.
|
On further discussion a pretty compelling argument came up that this assertions might not be necessary at all - I first considered that adding this purely as a migration aid for anyone using "to have keys satisfying", but when prodded I realised if I needed to something like this I'd probably write: expect(Object.keys(obj), 'to have items satisfying', 'to be a string');It seems like the deprecation stuff has more legs than the assertion itself, and as such I've split it off as a separate PR. |
I was not suggesting that we should remove that feature, I also use that a lot. I was just saying that we actually handle
I had the same reaction, but we are going to limit the use of |
|
@alexjeffburke I'm not wild about us just removing random stuff. I think @alexjeffburke and @papandreou maybe we should make a issue about deprecating these things, it seems like it isn't fully solidified, if we are going to break compatibility, then it has to be with a pretty clear path. |
|
@sunesimonsen @papandreou I very much apologise if it felt like suggesting random removals, certainly not me intention. It was totally the right call to write these up, if we intent to do something about this - hopefully the following will suffice: #690 |

Part of my push for Unexpected 12 is to have consistent behaviour regarding ignoring undefined - so far this has centred primarily around making sure the "properties" assertions can take over from the "keys" assertions (which are sensitive to undefined for legacy reasons).
I discovered one last missing assertion for there to be feature parity between properties and keys (thus allowing us to consider deprecation of keys are previously discussed), namely: "to have properties satisfying". This PR implements the missing assertion - the most important difference being ignoring keys with undefined values (https://github.com/unexpectedjs/unexpected/compare/feature/toHavePropertiesSatisfying?expand=1#diff-0a56a2b979f6080486ec1909508cda1aR526).