On Mon, Mar 7, 2016 at 12:02 AM, <[email protected]> wrote: >> 2nd it is not a real import and is already confusing > > I know it's not a real import but ansible made it look like a real import so > coding styles should apply. >
Like you I really hate wildcard imports. They make static analysis of the code harder and pollute the namespace. However... >> Your change makes it even more misleading as it implies a restricted >> import which is not true > > My change only makes a completely arbitrary line that we are forced to add > to comply to coding standards. The misleading part comes from making a line > that looks like an import to do something that is definitively not an import > : ) (I wonder how many people contributing to ansible know that line is > actually not an import). Brian's point is that the way module replacer works, the effect of a real "import *" is closer to what actually happens: all of the names defined in the other module are then available in the module and they overwrite other globals that were defined earlier in the module. Contrariwise doing an import AnsibleModule is farther from the truth as it makes things like the overwriting of global names less visible to someone who doesn't know about this being, essentially, a preprocessor directive instead of an actual import. So it's bad that ansible reuses a "from ansible.module_utils.<NAME> import *" line to mean "Insert the contents of <NAME>.py here" but it would be worse if ansible *also* reused "from ansible.module_utils.<NAME> import <FOO>" to mean the same thing. This is a case of doing one wrong thing being less bad than doing two wrong things. == Ways forward == Although we won't take your PR, there are two things that could make the problem you're describing better. First, I'm working on ziploader for 2.1. That will allow people to write modules that use actual python imports instead of these preprocessor directives. When I spec'd it out, I anticipated a lot of work because some things in module_utils and in actual modules assume that everything is in one file and therefore some things are going to be global. Those things won't work under ziploader and will either need special handling or porting in order to use ziploader versions of the module_utils modules. However, jimi-c wrote a proof-of-concept that will at least load module_utils/basic.py This week I'm going to be looking at whether that generically works around my worries about backwards compatibility or if it's just that basic.py is cleaner in this respect than other module snippets. then working on getting a POC merged into a branch of ansible/ansible/. My goal is to have at minimum, the ziploader framework in 2.1.0. If possible, to also have module_utils adapted to work with ziploader (as noted, this may require porting and changing the APIs of things in module_utils somewhat. If so, I'll probably have to create a new namespace for these module_utils to live within). Second, no one inside of ansible is working on this but bcoca and I have both been amenable to the idea that "from ansible.module_utils.<NAME> import *" is bad syntax. So we'd be willing to accept a pull request that added an alternative syntax. ansible already uses b"#<<INCLUDE_ANSIBLE_MODULE_COMMON>>" as an older way to include basic.py. We'd probably want to build on this syntax. Maybe something like b"#<<INCLUDE_ANSIBLE_MODULE basic.py>>" (where basic.py can be substituted with any python filename inside of the ansible/module_utils directory.) We aren't all thinking along the same lines (yet) about whether we'd want to use that alternate syntax inside of the modules we ship in core and extras or just allow it for third-party custom modules. I think we'll be better able to make a decision about that once we have ziploader in place and see whether we'll have many modules that have to use module_replacer to include snippets from module_utils or if almost everything works with ziploader. ziploader links: * jimi-c's proof of concept: https://github.com/ansible/ansible/compare/devel...jimi-c:module_utils_commmon_loading * Some sample code from me, mostly contained in jimi-c's POC: https://gist.github.com/abadger/d3592c1c9ef37ca54db0 * An earlier mailing list post I made on the subject: https://groups.google.com/forum/#!search/d3592c1c9ef37ca54db0/ansible-devel/Tv_9GXnDJRI/XitYR7erCwAJ -Toshio -- You received this message because you are subscribed to the Google Groups "Ansible Project" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/ansible-project/CAG9juEpn%3DdYfHwoGxRnHU2zA3KD33iibeCtRmBvshxhJKcDUgw%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
