simphony / simphony-common

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

Problem: implicit code inspection for metadata loading is unclear and complex #292

Closed mehdisadeghi closed 8 years ago

mehdisadeghi commented 8 years ago

Solution: make it explicit by adding a register decorator.

mehdisadeghi commented 8 years ago

@kitchoi @tuopuu Today I ran into another problem while loading extensions so I came up with a new solution. I added a new class decorator called 'register' and merged extension and engine packages (less clutter). Putting a register on top of a proper class will load its metadata. Please have a look, thanks.

kitchoi commented 8 years ago

@mehdisadeghi Are there now two ways to add an engine extension to an EngineManger? One by calling EngineManger.load_metadata(module), another by the register decorator which add the extension when the module is being loaded? What should happen if I have already loaded the module that contains an engine extension (i.e. registered to an EngineManager by the decorator) but then I want to replace the EngineManager instance with another EngineManager? (I think that's the intention, right?)

kitchoi commented 8 years ago

@mehdisadeghi I can't find where set_engine_manager is used. What's its use case? How do we retrieve the extensions registered to the EngineManager that is replaced by the set_engine_manager?

kitchoi commented 8 years ago

My comment regarding replacing the EngineManager after importing an extension-containing module can be shown in this test case here: https://github.com/simphony/simphony-common/blob/fix-cyclic-imports-experiment/simphony/engine/tests/test_register_engine_extension.py

mehdisadeghi commented 8 years ago

@kitchoi I wrote those tests and set_engine_manager having implicit load in mind, hence some inconsistency.

I'll update the code to better reflect the use of the decorator.

mehdisadeghi commented 8 years ago

@kitchoi Please checkout the latest changes. Feel free to add your changes in a commit or a PR.

kitchoi commented 8 years ago

Thanks @mehdisadeghi. The changes look good. There is an error in the tests, can you fix that please?

mehdisadeghi commented 8 years ago

I have already fixed that, will push it soon.

codecov-io commented 8 years ago

Current coverage is 96.45%

Merging #292 into master will decrease coverage by -<.01%

@@             master       #292   diff @@
==========================================
  Files            48         49     +1   
  Lines          3813       3808     -5   
  Methods           0          0          
  Messages          0          0          
  Branches        571        569     -2   
==========================================
- Hits           3680       3673     -7   
- Misses           44         45     +1   
- Partials         89         90     +1   

Powered by Codecov. Last updated by d1c878f...3e242ef

mehdisadeghi commented 8 years ago

@tuopuu @SGGgarcia @kitchoi @roigcarlo Please review and press the merge button if is OK!

kitchoi commented 8 years ago

Looks good to me