Common base class

[pekkaklarck]

Currently both DynamicCore and StaticCore extend HybridCore. This is a bit strange inheritance hierarchy in general, but it's especially stupid to check does a library extend any of these by using isinstance(library, HybridCore). It would be better to have a common base class named LibraryCore or RobotLibraryCore that all concrete lib cores extend. It would allow using isinstance(library, RobotLibraryCore).

Until this common base class is implemented, it's probably best to use isinstance(library, (HybridCore, DynamicCore, StaticCore)) with any generic code. That's both more explicit than just using isinstance(library, HybridCore) and also works if and when other cores don't anymore extend HybridCore.

[pekkaklarck]

One thing to decide is what functionality to move into the possible new RobotLibraryCore base class. Possible solutions:

1. Just create empty RobotLibraryCore class and change HybridCore to extend it. Others can then keep extending HybridCore but they have common RobotLibraryCore base class as well. Easy and fully backwards compatible. DynamicCore still extending HybridCore is strange.

2. Rename HybidcCore to RobotLibraryCore and then create new HybricCore that simply extends RobotLibraryCore. Other cores can then be changed to extend RobotLibraryCore as well. Easy to implement and DynamicCore not anymore extending HybridCore is good. Slightly backwards incompatible due to reasons explained in the original description.

3. Create new RobotLibraryCore class and move functionality related to adding library components to it. Leave functionality related to attributes that DynamicCore doesn't directly need to HybridCore. This means especially __getattr__ and __dir__, but possibly also code related to getting these attributes. Change all cores to extend RobotLibraryCore. Best separation of functionality, but it is possible that libraries using DynamicCore need these attributes even though DynamicCore itself doesn't need them.

Do I @aaltat remember correctly that 3. would be problematic to SeleniumLibrary? In that case I guess it's best to go with 2.

[aaltat]

Hmmm, I have this feeling that there was something, but I can not recall what that actually was. Also could not find anything by randomly looking at the SL code. I recall that we did have problem, when keyword decorator did change the name of keyword and then the method implementing the keyword was not anymore available in the SL public API. But that was fixed in the libcore side and can't recall the details.

[pekkaklarck]

Well, __getattr__ provides the ability to call exposed keywords directly using the main library like SeleniumLibrary().open_browser(). Removing that support from this project would require re-implementing it in SL. Clearly not worth it.

The problem you mentioned was most likely #2 and it was fixed on this side in 0559e5c.

[aaltat]

That was the problem what was lurking in my mind.

[pekkaklarck]

And it confirms option 3 is not a good idea.