BHoM / BHoM_Engine

Internal manipulation of the BHoM
GNU Lesser General Public License v3.0
26 stars 14 forks source link

Serialiser_Engine: Update NullOrEmptyCheck to just check for Null #3364

Closed peterjamesnugent closed 3 months ago

peterjamesnugent commented 3 months ago

Issues addressed by this PR

Closes #3357

Test files

Unit tests Overriden objects and non-overriden objects

Changelog

Additional comments

peterjamesnugent commented 3 months ago

@BHoMBot check compliance

bhombot-ci[bot] commented 3 months ago
@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 commented 3 months ago

@BHoMBot check unit-tests

bhombot-ci[bot] commented 3 months ago
@peterjamesnugent to confirm, the following actions are now queued: - check `unit-tests` There are 7 requests in the queue ahead of you.
peterjamesnugent commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

@BHoMBot check required

bhombot-ci[bot] commented 3 months ago
@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 commented 3 months ago

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

bhombot-ci[bot] commented 3 months ago
@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 commented 3 months ago

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

bhombot-ci[bot] commented 3 months ago
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 commented 3 months ago

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

bhombot-ci[bot] commented 3 months ago
@pawelbaran I have now provided a passing check on reference `26892311972` as requested.
pawelbaran commented 3 months ago

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[bot] commented 3 months ago
@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](https://github.com/BHoM/documentation/wiki/Continuous-Integration).
pawelbaran commented 3 months ago

@BHoMBot check ready-to-merge

bhombot-ci[bot] commented 3 months ago
@pawelbaran to confirm, the following actions are now queued: - check `ready-to-merge`