simphony / simphony-common

The native implementation of the Simphony cuds objects
BSD 2-Clause "Simplified" License
7 stars 5 forks source link

Problem: dynamically created data containers have bad base classes #319

Closed mehdisadeghi closed 8 years ago

mehdisadeghi commented 8 years ago

Any class created with create_data_container helper function, which is supposed to be subclass of DataContainer, will not pass the following check:

DC = create_data_container()
dc = DC()
isinstance(dc, DataContainer) # False

Solution: add __bases__ attribute to the created classes

codecov-io commented 8 years ago

Current coverage is 72.04% (diff: 100%)

Merging #319 into master will not change coverage

@@             master       #319   diff @@
==========================================
  Files           107        107          
  Lines          6810       6810          
  Methods           0          0          
  Messages          0          0          
  Branches        806        806          
==========================================
  Hits           4906       4906          
  Misses         1867       1867          
  Partials         37         37          

Powered by Codecov. Last update 4e6f557...04616ef

itziakos commented 8 years ago

I do not think that there is a need for the check isinstance(DataContainers). It is actualy wrong to do so since wrapper engines could have their own implementation of the DataContainer object. The important thing is to support the api for all CUDS objects as described in the docs.

Furthermore, manually manipulating __base__ is beyond bad. If the need for an isinstance check is anavoidable the we need to have ABCDataContainer where compatible classes will register themselves and then isinstance(ABCDataContainer) will be true.

itziakos commented 8 years ago

Furthermore, manually manipulating __base__ is beyond bad

Hmm.. I missed the fact that you are already creating a class at runtime. However, looking at the code DataContainer should be already part of the base classes since the type function should have done it for you.

mehdisadeghi commented 8 years ago

DataContainer should be already part of the base classes since the type function should have done it for you

Strange. On my system it didn't work without explicitly setting __bases__. I'll double check if I'm missing something.

itziakos commented 8 years ago

I think that there is a confusion. The result of the create_data_container is a class type. when checking with isinstance you should check with result()

mehdisadeghi commented 8 years ago

you should check with result()

I checked with the instance. However, as you referenced the type doc, it should be already done by setting the bases tuple. I have definitely made a mistake during my debug. I'll probably close this PR.

itziakos commented 8 years ago

I have definitely made a mistake during my debug.

Yes I wanted to say that the instance, class type confusion could be done during the debug session.

mehdisadeghi commented 8 years ago

This PR is misleading, I close it.