How open are you to code-refactoring Pull Requests?

[tcoyvwac]

Hi there @vitoplantamura , love your project a lot! 😄

As titled, I wanted to contribute and send some Pull Requests, however I am slightly unsure due to the unique design of your codebase.

Most of my Pull Requests are about standardising C++ projects. To list some examples, creating some more defined translation units, so header files and source files. This means extracting code-fragments and elements into a more modular translation-unit design instead of maybe currently "onnxstream.{cpp,h}" hoarding all-the-things 🥲 . This more modular aim would help a lot in compile times of the project, as well as making the line-border between "library code" / "application-client" code much more clearer + cleaner. This will also mean Pull Requests will be adding more "functions" to break up and clearer define some longer pieces of your code, as well as adding some extra namespacing, data types and use of already-in-the-standard C++ library code to reduce the size of the OnnxStream codebase quite a bit.

You have done a lot @vitoplantamura to create and maintain OnnxStream and it seems that you have a really awesome development flow right now! I can kinda see how this codebase grew and was built into its unique form currently, and as I am a fan of the project, I don't want to slow your flow down, yet want to make OnnxStream more standardised for other C++ / software developer fans (like me!) of your project!

Do you have any restrictions or issues with any codebase refactoring to OnnxStream? Or any suggestions before filing any Pull Requests to refactor some of your project's code?

Thanks for reading @vitoplantamura and to all your upcoming updates, cheers! 🥳

[vitoplantamura]

hi, Thanks for opening this issue before sending any refactoring PR. I am against PR of this kind for two reasons: 1) the fact of having a single implementation file and a single header file is included among the features of the project :-) the objective is to "force" an implementation as compact as possible (I guess at the expense of the readability of the code and, marginally, of the compilation times). 2) any refactoring activity could introduce bugs that may be difficult to detect. Since no unit tests have been written for the library, a bug in an operator (for example) may be difficult to spot (I say this from experience!). This risk would be acceptable if we were talking about introducing a new key feature. Thanks, Vito

[tcoyvwac]

Hi there @vitoplantamura,

So, I totally get + and really agree about the need to make this project's implementation as compact as possible. 😄
Like you mentioned in your first point, there are many benefits a unity-build styled project ¹ has to offer and I understand if this is the preferred approach for OnnxStream. 👍

These benefits however are not seemed to be used so far to its best intentions + potentials... For example 🔍 , there is currently a very large monolithic class declaration / definition used in the project. Maybe I am misunderstanding this monolithic-class's intention, but all private functions of this large monolithic class must be visible to other translation units for overload resolution + linking reasons. A unity-build style would probably use the unnamed-namespace more (or similar alternatives) as the standard way of hiding detail to other translation-units. This would certainly require extracting various code-fragments from this monolithic class into more streamlined free-form / independent functions.

I totally understand and agree with you that testing is very important. So for your second point, as help for all contributors and based on your past experience testing OnnxStream, could you share your testing suite which you use to catch your difficult-to-spot bugs? 🔍 🪲
This would be very helpful + useful for anyone who would like to contribute code to be merged to Onnxstream.

In the end, we do align a lot with goals & objectives - in addition to testing benefits + reasons - fostering an implementation as compact and concise as possible for OnnxStream. 🤝 ⭐ I hope you can understand why I do recommend using standard C++ practice and especially the C++ standard library and functions - to save a lot of duplicated + "reinventing-the-wheel" produced code, making the project's code-size small and all the additional bonus benefits standardisation has to offer.

Would you accept more standardising unity-build friendly refactoring Pull Requests for OnnxStream? This would keep to your main focus of the project you wanted to highlight in point 1 - in addition would also improve the standard code readability + the compile times of this project. 😉 👍

Thanks and hope this helps,

Footnotes

1. https://en.wikipedia.org/wiki/Unity_build ↩