ADR Suggestion Criteria for Moving Functionality into corelib

[AndrewSazonov]

In another ADR suggestion #194, we touched the question of when something should go into corelib. One opinion that came up is that functionality should only be added to corelib when it is needed by all dependent projects. This is how I understood the following comments:

Since neither refl nor imaging have any use for name in their parameters, having this attribute in the base class makes little sense, I think.

and also

We shouldn't add properties or methods to the core lib which aren't useful to everyone.

This logic sounds reasonable. However, it raises a practical question: how exactly do we define this limit in real situations? Is it really only when all dependent projects need it?

Let’s imagine a simple case with four main technique-specific projects. Two of them need an attribute like name, and two of them do not. In this situation, the suggestion seems to be to keep it out of corelib. Fine. But later the situation changes: now three projects need this attribute, while only one still does not. In this case, do we still keep it out of corelib?

In that case, we would end up with three separate implementations of the same concept in three different projects. Of course, adding a single attribute to a base class is not difficult, and each project can do it on its own. But this could creates some risks. For example:

• The same idea may be implemented slightly differently in each project.
• Attributes like, e.g., pretty_name or similar may get different meanings аnd applications.
• This can become confusing when reviewing code across projects or moving between them.

Now imagine something more serious than a single attribute. For example, a shared utility method, such as, e.g., fetching tutorials from a project repository. If one project does not need it, but three others do, then those three projects would likely implement and maintain their own versions. This duplicates effort and makes long-term maintenance harder. Maybe this example is not perfect, but I hope it explains my concern.

Because of this, I tend to think that if at least two projects share the same need, it may already make sense to consider moving that functionality into corelib. This would:

• Reduce duplicated implementations.
• Improve consistency across projects.
• Make maintenance and review easier.

In practice, developers of those two projects would need to implement and maintain this functionality anyway. So why not join the effort and do this work together? And if it is developed together, where else this shared code should live, if not in corelib?

So, from my perspective, maintaining shared functionality in one place for more than one (!) project feels simpler and safer than maintaining similar solutions separately in each project. It is like applying the DRY rule across projects, not just inside one project.

[henrikjacobsenfys]

I agree and have nothing to add.

[damskii9992]

I am glad that you brought up DRY. Because I personally think WET programming is much more readable and maintainable than DRY code. I don't believe that, simply because 2 repos share functionality, that it makes the maintenance and review easier. In fact, I believe that overdoing the DRY concept makes a codebase extremely convoluted, complicated and difficult to read, understand and thus maintain.

For example, according to your suggestion, we should move all of EasyCrystallography into corelib simply because it is shared between Diffraction and Imaging?

Obviously it is not so cut and dry that we can simply apply any given fraction agreement to any piece of code. There is a big difference in how destructive/annoying it is to incorporate certain code into corelib to the techniques which don't use it.

For example, adding a new class, like the ModelCollection, even though it might not be used in EasyImaging, it doesn't matter because I simply won't use the class. However, adding a new "name" attribute to our base classes means I HAVE to have that attribute in EasyImaging, despite thinking it makes for a much more confusing user API. So clearly different things should be treated differently, and everything should be on a case-by-case basis.

It does however make sense to maybe make an ADR on what to do in disagreements, like with this name attribute. And in such a case, I think what makes most sense is to let the majority decide. If its a 50/50 split, then we don't implement it since there is no majority. That is my opinion on it. So in the case of adding an attribute to the base classes, we only do it if 3/4 techniques want it.