Is not empty should be false when a node is missing

[ashleyfrieze]

Possible fix for #53

@lmartelli - please review and let me know what you think?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.58%. Comparing base (a396861) to head (cf1f8c3).
Report is 1 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main      #54      +/-   ##
============================================
+ Coverage     97.54%   97.58%   +0.03%     
- Complexity      490      496       +6     
============================================
  Files            64       64              
  Lines           856      869      +13     
  Branches         64       64              
============================================
+ Hits            835      848      +13     
  Misses           12       12              
  Partials          9        9              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lmartelli]

It does the job for missing nodes, but what about null values ? It feels wrong that assertJson("{\"prop\": null}").at("/prop").isNotEmpty(); passes.

Suggested change

	return satisfies(not(MissingCondition.getInstance()).and(not(new IsEmpty())));
	return satisfies(not(MissingCondition.getInstance())..and(not(NullCondition.getInstance())).and(not(new IsEmpty())));

[lmartelli]

@test
void isNotEmptyOnNullIsFalse() {
assertAllWays("{"count":0}", "{"count": null}",
assertion -> assertion.at("/count").isNotEmpty());
}

[ashleyfrieze]

@lmartelli - I think you're right. I've pushed another extension to the code. See what you think.

[lmartelli]

Looks good to me. But I wonder if it's a good think to allow checking for emptyness for all value types. It does not really mean anything for booleans or integers.

[ashleyfrieze]

@lmartelli - given that everything in JSON is an any, when we're asking isEmpty or isNotEmpty on a node of any type, it's on us to determine if that's meaningful.

I'd be happy that isEmpty is true for things where emptiness is a thing - e.g. strings, arrays and objects, and that isNotEmpty is both a truthy AND emptiness checker.

Realistically, it's on the user not to use empty to check things where emptiness has no meaning.

Have you a counter proposal? By the way, I'm appreciating having someone to discuss these ideas with and shape the tool.

[lmartelli]

I encountered the issue because I used isNotEmpty() on an expected string. In the end, I used isNotEmptyText() which was better suited to my needs.

I am not very comfortable with isEmpty/isNotEmpty being both a truthy and emptiness checker. Better to be explicit about what you expect. I would deprecate isEmpty() and provide isEmptyArray() and isEmptyObject().

[ashleyfrieze]

@lmartelli I took the spirit of your point and I've implemented something deeper. I also found out that the README recommended the use of more assertions than just isEmpty.

It recommended:

assertJson(foo)
   .at("/path").isText()
   .at("/path").isEmpty();

or

assertJson(foo)
   .at("/path").isText()
   .at("/path").isNotEmpty();

I've now made it so that .text(), .array() and .object() - which specialise the DSL down to things relevant to those types - also assert that you're in the right type.

This means that the preferred route is:

assertJson(foo)
   .at("/path").text().isNotEmpty();

And the isNotEmpty here is not doing any special check other than !empty.

But I've left the shortcuts in place, with a deprecation on isNotEmpty when done generically.

I think this allows the shorthand, which is convenient, but also more heavily recommends a stronger-typed approach, which is clearer.

I don't like isEmptyText as much.

I need to do this for Boolean and Numeric node DSLs too... but give me early feedback.

[ashleyfrieze]

@lmartelli - thanks for your input. I've done some further refactoring to make this new approach make sense. This might be a breaking change for some users. I'll get this released in due course.