avocado-framework / avocado-vt

Avocado VT Plugin
https://avocado-vt.readthedocs.org/
Other
84 stars 243 forks source link

Risk of circular imports #2778

Open smitterl opened 4 years ago

smitterl commented 4 years ago

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?

chunfuwen commented 4 years ago

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

pevogam commented 4 years ago

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.

dzhengfy commented 3 years ago

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.