Serialiser_Engine: Update NullOrEmptyCheck to just check for Null

[peterjamesnugent]

Issues addressed by this PR

Closes #3357

Test files

Unit tests
Overriden objects and non-overriden objects

Changelog

• Updated NullOrEmptyCheck in Serialise method for IBHoMObjects to just check null for Name properties;
• Updated unit tests for CellularSectionFromBase following changes to the Serialse method;

Additional comments

[peterjamesnugent]

@BHoMBot check compliance

[bhombot-ci]

@peterjamesnugent to confirm, the following actions are now queued:

• check code-compliance
• check documentation-compliance
• check project-compliance
• check branch-compliance
• check dataset-compliance
• check copyright-compliance

[peterjamesnugent]

@BHoMBot check unit-tests

[bhombot-ci]

@peterjamesnugent to confirm, the following actions are now queued:

• check unit-tests

There are 7 requests in the queue ahead of you.

[IsakNaslundBh]

I think the changes here most likely makes sense.

Caveat would be if we could end up with a similar issue again as the one you have just fixed, where we had some data that have name set to empty string as default for the class, then for whatever reason the name as been set to null, then the object serialised. When de-serialised than, the name will be skipped, and you will end up with a miss-match compared to the original object, where the de-serialised object will have the name set to the default value (most likely an empty string) but the original object having its name set to null.

A bit on the fence on this one, and the best way to handle it tbh. Could complicate the check by only including if it is not null or empty AND not equal to the default value for the class. This ofc makes the check more complex, but only way I can think of that guards against the problem above. Then you still have the edge case of someone changing the default value of the class, but that is a case I think we could live with.

[pawelbaran]

Actually, I second to the comment of @IsakNaslundBh - dealing with name may lead to issues in case the default value is overridden (see script here, all overrides can be quickly looked up as here). Also in the proposed shape, values equal to null get deserialised to empty, so FromJson != ToJson (see code comment with a fix and test in script above - however, it still does not address the general problem of overridden defaults).

So to summarise, personally I would rather always serialise the name as an optimal tradeoff between risk/effort and volume/performance (storing an empty name is literally a few characters in total).

[peterjamesnugent]

Added the check suggested by @pawelbaran as that solves the majority of cases serialising/deserialising. This does leave a mismatch for overriden properties though as commented above.

Perhaps we should add an additional check for unit-tests that looks for null names or any string properties that is null so they can be updated rather than have to patch over the Serialise and Deserialise methods?

[pawelbaran]

Are you against removing case "Name" from the code altogether @peterjamesnugent? Maybe it got lost in our lengthy comments, but I think both @IsakNaslundBh as well as me opted for such solution - would be good to understand why do you still keep it 👍

[pawelbaran]

Happy to approve the code changes, I think what we have now is an optimal trade-off. I see 2 alternatives here:

• allowing for bugs/inconsistencies we are aware of (in case of overridden default) - we ruled it out, not great
• checking whether value.Name is a default value of the property - I did quick research and it looks like this would require instantiating a dummy object, which is a massive overhead plus potential risk (think of IImmutable)

...so I think what we have now is best value that we can get for now, better alternatives to be rather searched in a separate thread 👍

[adecler]

I would follow Ockham on this one. If removing code solves the problem, this is probably a good direction to consider.
More seriously, the exclusion of some properties from json was purely a minor but straightforward optimisation. If it stops being straightforward, then the best approach is indeed to remove it.

Some quick test confirms that this change

• doesn't have a significant impact on the size of the json (BHoM_Guid, _t, and _bhomVersion alone are already much longer)
• does maintain the name value even when it is null

Although more testing is always good, I don't see any reason to have anymore than this simple one as long as the bot checks are passing so happy to approve.

[pawelbaran]

@BHoMBot check required

[bhombot-ci]

@pawelbaran to confirm, the following actions are now queued:

• check code-compliance
• check documentation-compliance
• check project-compliance
• check core
• check null-handling
• check serialisation
• check versioning
• check installer

[pawelbaran]

@BHoMBot check copyright-compliance
@BHoMBot check dataset-compliance
@BHoMBot check unit-tests

[bhombot-ci]

@pawelbaran to confirm, the following actions are now queued:

• check copyright-compliance
• check dataset-compliance
• check unit-tests

There are 17 requests in the queue ahead of you.

[pawelbaran]

@peterjamesnugent do you have any clue if the unit tests failure is caused by this PR?

[bhombot-ci]

FAO: @FraserGreenroyd
@pawelbaran is seeking dispensation on this Pull Request to skip a required check. Please can you provide authorisation for the check to be skipped, or provide assistance as appropriate.

The check they wish to have dispensation on is unit-tests.

If you are providing dispensation on this occasion, please reply with:

@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 26892311972

[pawelbaran]

@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 26892311972

[bhombot-ci]

@pawelbaran I have now provided a passing check on reference 26892311972 as requested.

[pawelbaran]

Dispensated unit tests check on the basis of the fact that the errors (here) are exactly the same as in other BHoM_Engine PRs (e.g. here).

[bhombot-ci]

@pawelbaran sorry, I didn't understand.
Was that comment an instruction for me? If so, could you state again what check you would like me to do?
For a list of available instructions, please see this wiki page.

[pawelbaran]

@BHoMBot check ready-to-merge

[bhombot-ci]

@pawelbaran to confirm, the following actions are now queued:

• check ready-to-merge