avocado-vt icon indicating copy to clipboard operation
avocado-vt copied to clipboard

Risk of circular imports

Open smitterl opened this issue 4 years ago • 3 comments

Based on discussion https://github.com/avocado-framework/avocado-vt/pull/2772#discussion_r496585978

There seem to be a risk of circular imports in avocado-vt. One simple way of preventing circular imports could be:

R: Allow a new avocado-vt module to import either a. from avocado b. from avocado-vt if the depth of the importing module is greater than the one of the imported module.

Examples:

  1. virttest.utils_misc has depth 2, virttest.utils_libvirt has depth 2; therefore, utils_libvirt/init.py is not allowed to import from utils_misc
  2. virttest.utils_libvirt.libvirt_network has depth 3, therefore it can import from utils_misc and from utils_libvirt

For existing modules, e.g. utils_misc, however, there could be issues: e.g. utils_misc (https://github.com/avocado-framework/avocado-vt/blob/master/virttest/utils_misc.py#L79):a) imports from virttest (that's okay because depth(virtest) > depth(virttest.utils_misc)); it imports from virttest.staging, virttest.xml_utils which is on the same depth and therefore might allow for circular imports.

By following rule R, new modules won't possibly cause circular imports because a. if moduleA imports moduleB (both in avocado-vt) ==> depth(moduleA) > depth(moduleB), then R prevents moduleB from importing moduleA b. moduleA can't be in avocado, and avocado doesn't import from avocado-vt.

@chunfuwen What do you think?

smitterl avatar Sep 29 '20 14:09 smitterl

@sathnaga @kylazhang @dzhengfy what's your thoughts about this?

chunfuwen avatar Sep 30 '20 02:09 chunfuwen

I think this is an interesting idea and we should give it a try. Could it be properly documented somewhere, e.g. in the contribution guidelines? All maintainers have to be made aware of it. We can gradually move in this direction as it won't happen overnight.

pevogam avatar Nov 03 '20 21:11 pevogam

I totally agree with @smitterl 's summary. Actually we were also discussed within the team. This folder, 'virttest.utils_libvirt', was just initiated to be created on that purpose. We will create more modules with depth 3 by features under this folder.

Thanks.

dzhengfy avatar Nov 16 '20 03:11 dzhengfy