Add ObjectPool reading, writing and manipulations

[Thom-de-Jong]

First attempt to implement .iop file reading, parsing and writing.
ObjectPools can also be changed or constructed in code.

TODO: implement the Serde crate.

Contains some dependencies on the changes to name from #22

[github-actions]

File	Coverage	Lines	Branches
All files	30%	30%	0%	✅
src/object_pool/object_pool.rs	0%	0%	0%	✅

Minimum allowed coverage is 0%

Generated by 🐒 cobertura-action against 7e24191

[ad3154]

Some of these things it would be amazing if we could fully unit test - like give it a real object pool and validate each object and each objects' components. Unsure if that's something we'd want to do now or later, as that seems like a fair bit of work.

Generally this all seems pretty good for manipulating a pool, reading, and writing. I suspect more than anything that people would bring a binary object pool, but in the case that we write a VT server or want to programmatically manipulate or generate a pool, then these object layouts will be very nice to have.

[Thom-de-Jong]

Yea... copy/ pasted it from my OpenIsobus project.
I have not thought about the copyright and unit tests.

I will fix those and do a bit more cleanup.

[JannesBrands]

I started to integrate some unit tests and continued the work of @Thom-de-Jong so that it compiles and without the todo! it even pass the test. So it seems to work so far loading the iop.
https://github.com/JannesBrands/AgIsoStack-rs/commit/df5dfe9a828d84dc40346fc4d69e1b0c8d0230af

[ad3154]

I started to integrate some unit tests and continued the work of @Thom-de-Jong so that it compiles and without the todo! it even pass the test. So it seems to work so far loading the iop. https://github.com/Open-Agriculture/AgIsoStack-rs/commit/df5dfe9a828d84dc40346fc4d69e1b0c8d0230af

This link doesn't seem to be working for me 🤔

[JannesBrands]

I started to integrate some unit tests and continued the work of @Thom-de-Jong so that it compiles and without the todo! it even pass the test. So it seems to work so far loading the iop. https://github.com/Open-Agriculture/AgIsoStack-rs/commit/df5dfe9a828d84dc40346fc4d69e1b0c8d0230af

This link doesn't seem to be working for me 🤔

Ahh, I don't know what link I put before. But the replacement should work.

[JannesBrands]

Here my continuation for this night:
JannesBrands@aa83d3f

[JannesBrands]

Here my continuation for this night:
JannesBrands@a94edaf

[JannesBrands]

I got stuck on the validation of the Graphic Context and its resulting consequence on reading the pool. I've wasted too much time racking my brains and have finally found out that the Pool Designer has a bug. Bug report is out and tomorrow I will continue.
JannesBrands@6c01903

[ad3154]

I got stuck on the validation of the Graphic Context and its resulting consequence on reading the pool. I've wasted too much time racking my brains and have finally found out that the Pool Designer has a bug. Bug report is out and tomorrow I will continue. JannesBrands@6c01903

Thanks for your continued work on this. Busy time at work right now, but we've got another innovation sprint coming up near the end of the year where I hope to be working full time on this project again.

[GwnDaan]

I got stuck on the validation of the Graphic Context and its resulting consequence on reading the pool. I've wasted too much time racking my brains and have finally found out that the Pool Designer has a bug. Bug report is out and tomorrow I will continue.

Ahh, that sounds like a bummer! I will also be able to work more on this again in the coming months.

I just got a chance to look at your additions, overall good stuff! There's one thing I noticed, and also involves your issue, is that you are reading a IOP file to test all objects. I feel like doing it this way might result in headaches later down the line when maintaining the unit test. We might run into a similar issue you are facing right now, and furthermore it is not really easy to expand with more objects.

My suggestion: unit test the objects directly from a hex string/array. E.g. pass the hex array directly to the de-serialization method for each object individually, and then test it's contents. Then the other way around can be unit tested easily and maintainable as well, by simply serializing the same 'de-serialized' object again and checking if it equals the defined hex array used at the start.

I'm sure we can do a small unit test for the from_iop() that maybe checks the number of objects present in the parsed pool, but doesn't have to check the contents of it. As that should really be done somewhere else in smaller parts IMO. That said, I'd love to hear your thoughts, and also of others.

I also suggest making a draft PR into @Thom-de-Jong's tjd/object-pool branch such that your changes are easier to follow and we have a dedicated place for any discussion/suggestions about these changes.

Last but not least, I want to say thanks for continuing to put effort into this 🚀

[JannesBrands]

My suggestion: unit test the objects directly from a hex string/array. E.g. pass the hex array directly to the de-serialization method for each object individually, and then test it's contents. Then the other way around can be unit tested easily and maintainable as well, by simply serializing the same 'de-serialized' object again and checking if it equals the defined hex array used at the start.

Yep, completely agree on that. Just trying to write it down as fast as possible cause its a kinda annoying work.

[codecov]

Codecov Report

Attention: 2028 lines in your changes are missing coverage. Please review.

Comparison is base (44006ea) 40.67% compared to head (8e9c96a) 11.52%.

Files	Patch %	Lines
src/object_pool/reader.rs	6.16%	654 Missing ⚠️
src/object_pool/writer.rs	0.00%	622 Missing ⚠️
src/object_pool/object_attributes.rs	0.00%	387 Missing ⚠️
src/object_pool/object.rs	2.70%	108 Missing ⚠️
src/object_pool/object_type.rs	2.91%	100 Missing ⚠️
src/object_pool/object_pool.rs	12.50%	91 Missing ⚠️
src/object_pool/object_id.rs	20.58%	27 Missing ⚠️
src/object_pool/vt_version.rs	0.00%	19 Missing ⚠️
src/object_pool/colour.rs	18.18%	18 Missing ⚠️
src/network_management/name.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main      #23       +/-   ##
===========================================
- Coverage   40.67%   11.52%   -29.16%     
===========================================
  Files           8       17        +9     
  Lines         563     2664     +2101     
===========================================
+ Hits          229      307       +78     
- Misses        334     2357     +2023     

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

[GwnDaan]

I want to merge this into main, so we can all iterate on it faster and not linger on this large PR. Does that sound good?

[ad3154]

Works for me!

[Notgnoshi]

I've approved so that you can merge, but it appears the repo merge policy is set to "rebase and merge", but this MR doesn't fast-forward rebase (hence the "Merge branch 'main' into tjd/object-pool" commit at the HEAD.

I think there's two resolutions:

1. Have @ad3154 set the repo merge policy to allow merge commits
2. Rebase tjd/object-pool onto main (which, for the size of this branch, will be pretty painful)

---

As an aside, while I use a somewhat strict fast-forward only merge policy at work (and somewhat religiously believe it's the "right" way), I suspect we should have a pragmatically looser policy here, because I don't want to discourage contributions, especially from new contributors.

[GwnDaan]

Yeah, I agree. I initially set the repo merge policy to fast-forward merges only. I used the squash and merge for this PR to not bloat the commit history, but I agree that it's probably better to allow merge commits as well to not discourage contributions. I'll change it