Origen-SDK / o2

MIT License
4 stars 0 forks source link

Some Rust Internal Tester Cleanup #127

Closed ginty closed 3 years ago

ginty commented 3 years ago

I wanted to add a SupportedTester assignment to the tester classes in origen/src/testers so that when getting a tester object from the current targets it could be resolved to its test program model.

This led me down a rabbit hole of cleaning up some last remaining string-based tester identifacation (still might be the odd bit of it in there, but not much) and also a bit of refactoring of the tester traits. I thought we should continue the approach of regularly merging stuff like this rather than sitting on it for ages while I complete the prog gen work.

There's a few bits of new prog gen code in here and a lot of it is just minor renaming/type changing from Strings to SupportedTester values.

Most interesting is the updates to the tester traits:

https://github.com/Origen-SDK/o2/blob/0dba49f4d84327043e8c8ac17ca5e1aa67023d37/rust/origen/src/core/tester.rs#L552-L560

https://github.com/Origen-SDK/o2/blob/0dba49f4d84327043e8c8ac17ca5e1aa67023d37/rust/origen/src/testers/vector_based/mod.rs#L72-L82

coreyeng commented 3 years ago

This looks good! Glad the tester traits can stick around as I've found them useful too, but were definitely only oriented towards what I needed at the time.

I thought we should continue the approach of regularly merging stuff like this rather than sitting on it for ages while I complete the prog gen work.

1000% agree. These smaller bit-sized chunks are much easier to digest, even if some WiP stuff comes along for the ride.

This requires use of the unstable specialization Rust feature, but since we are already dependent on that via Pyo3 it doesn't seem like any risk to use it too.

Actually, as of Pyo3 11.0, unstable rust isn't required anymore. I meant to go see if we had any unstable stuff when I updated it but that PR got way out of hand and that particular thing went out the window.

I don't have a problem with needing unstable stuff, but not sure if we should move towards a stable Rust branch or not. Since it all ends up compiled anyway, I don't think it matters to the end users (provided the unstable stuff we use is working correctly).

ginty commented 3 years ago

Thanks for the review @coreyeng.

I didn't realize that about Pyo3, that's good, but does raise a question over whether we should then use unstable features or not.

This specialization feature is actually really useful if you want to develop in an oo style. I think let's keep it for now and we can always back out later if we need to without too much hassle. That would just mean that any classes which want to override some of the default trait functions would have to implement them all - so it would be a case of copying and pasting the defaults into any partial (specialized) trait implementations.

Probably going forward we should try to keep traits pretty small and specific, rather than having large traits with a lot of functions, that would naturally avoid the situation where you want to override only 1 trait function but keep the other 20.

Come to think of it most of the 3rd party traits I've implemented till now have been 1-3 function kind of size, this must be why you would want to do that.