BHoM / BHoM_Adapter

GNU Lesser General Public License v3.0
7 stars 6 forks source link

`Create()` and `Read()` to be Virtual instead of Abstract? #146

Closed alelom closed 4 years ago

alelom commented 5 years ago

I see the point of having Create() and Read() as abstract.

In theory, that forces developers in sticking to the "Replace" or CRUD paradigm, or at least to give a thought to that.

However:

  1. I think that in practice this is not true. You still need developers to read the Wiki to know how to properly implement and use the CRUD methods.

  2. In a way, I think that enforcing the implementation of Create() and Read() is giving a false sense of compliance. People still can (and will) just implement empty methods.

  3. It does not allow for flexibility. There are Toolkits where it does not make sense to enforce the "Replace" or CRUD paradigm. DataViz_Toolkit, GitHub_Toolkit, MachineLearning_Toolkit, Sharepoint_Toolkit, and the list goes on. Those toolkits are forced in implementing empty Create and Read methods, while they sometimes implement the "non mandatory" CRUD methods (Delete, etc).

  4. It conflicts with the fact that we give the possibility to override the Push. If you override the Push, nothing stops you to avoid using the Create and Read, which happens for all Toolkits at point 3. People just put empty Create and Read methods overrides in their Toolkits.

Proposal

@adecler @al-fisher @IsakNaslundBh @epignatelli @rwemay

IsakNaslundBh commented 5 years ago

Could argue both ways here, and the Template solves much of the reason for forcing implementation by making read and create abstract. That being said, I think it still can be useful to keep them, if for nothing else, to direct the intended default workflow of the adapter.

I do not really agree with it being a conflict of push/pull being override-able and create/read abstract. Abstract -> intended use, virtual -> you are still free to do whatever you want.

To me this is not a big thing though. If I had to choose I would probably keep them abstract to highlight their significance in the intend work of the adapter, but not too bothered if we make them virtual.

adecler commented 5 years ago

I agree with Isak.

alelom commented 4 years ago

This has been closed by #151