simphony / simphony-common

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

Problem: Cyclic import if loading extensions prior to simphony itself #291

Closed mehdisadeghi closed 8 years ago

mehdisadeghi commented 8 years ago

Solution: re-organize packages and fix #290.

mehdisadeghi commented 8 years ago

I realized if we import the extension package prior to the simphony itself a cyclic import will cause a failure in loading engine metadata information. I moved dependencies to ext package.

kitchoi commented 8 years ago

Another suggestion I would make would be to put ext under the engine directory, and instead of calling ext, call it extension. This would be more clear especially when you want to import create_wrapper, you would be calling engine.extension.create_wrapper, whose purpose can be hinted by the namespace.

Looks fine to me (assuming the tests are fixed, which looks to me simple, but I could be wrong)

kitchoi commented 8 years ago

Another alternative would be to move imports to the module-level functions. This is generally justified by having to avoid import cycles.

codecov-io commented 8 years ago

Current coverage is 96.51%

Merging #291 into master will increase coverage by +<.01%

@@             master       #291   diff @@
==========================================
  Files            47         48     +1   
  Lines          3806       3814     +8   
  Methods           0          0          
  Messages          0          0          
  Branches        571        571          
==========================================
+ Hits           3673       3681     +8   
  Misses           44         44          
  Partials         89         89          

Powered by Codecov. Last updated by acc8d52...9ba72a6

mehdisadeghi commented 8 years ago

put ext under the engine directory

This will not solve the issue. Loading extensions happen upon importing engine which executes engine.__init__. When an extension imports anything from engine package, the load_metadata method fails, because the classes inside the extension module are not yet defined.

Another alternative would be to move imports to the module-level functions

Yes this is possible. However, it implies relying on extension developers to do this (because this only happens when the first import is triggered from within an extension), which I prefer to avoid.

instead of calling ext, call it extension

I will rename the package.

kitchoi commented 8 years ago

I think the one thing that is blocking you is the create_wrapper, which is in both extension.__init__ and engine.__init__. I see you want to expose the function to the public API, is it possible to have it in one place and not in both?

mehdisadeghi commented 8 years ago

one thing that is blocking you is the create_wrapper

I removed that. It is not necessary (and not blocking anything either).

kitchoi commented 8 years ago

Looks good! :+1: