I understand that line is not a real import and you replace it with ansible code so you can ship a single file to the remote host (which the sanity of the idea could be argued but it’s not the point of this thread). However, you made it look like a real import that breaks PEP8 so I don’t understand why my PR was rejected without further discussion. Specially when the patch is so simple and transparent and allows people to write PEP8 compliant code.
First, its not a pep8 violation, having the import at the bottom is but that can be considered again as ‘not a violation’ as it improves redability when errors happen (line number line up).
2nd it is not a real import and is already confusing, I personally much preferred the << MODULES_BASIC >> way as it was clear that this is NOT a python import. Your change makes it even more misleading as it implies a restricted import which is not true.
And this is bad. This can alienate new developers. I like Python because it have formatting standards. When I open Ansible source files in my VIM with installed vim-syntastic, I see too much bad formatting. And I begin formatting fixing at first, instead of writing/fixing code.
Please, let us possibility to fix formatting issues.
having the import at the bottom is but that can be considered again as ‘not a violation’
That’s not the issue I am trying to solve with that PR.
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.
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).
Last, but not least, we don’t enforce PEP8 nor are particularly keen on following it blindly as many others do.
You can follow any coding style that you prefer. It’s your decision. I decided to write PEP8 compliant code and you are forcing me to write code that is not compliant by forcing me to write a wildcard import.
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.
We're hoping to have 2.1 out in late April or May (3-4 months from
2.0's release).
I hope to land code in a publically available branch by next week.
There are some things about it after that I think we (core committers)
will need to discuss and come to a consensus on before it gets merged
into devel.
Ok, let me know if I can help testing or providing feedback. I am really interesting on this as I have a lot of modules that I wrote with duplicated code. Being able to share code amongst them is going to be a huge win.
After going down a rabbit hole of related bugs, I've had some time to
work on jimi-c's initial branch. The branch I have now[1]_ is passing
unittests and integration tests (at least through mysql... I have an
issue on my system that's preventing mysql from passing and I've been
too busy coding to address that).
There's further feature work to do on this so don't take it as
finalized. Feel free to complain about things that don't work
The very best way to reach me is to find me on irc.freenode.net
abadger1999 in ansible-devel.
Things that are working now:
* Modules tested via the integration test suite seem to be working.
Other modules (including user's custom modules) are probably working
but I did have to make some changes to the module_utils code that some
of those rely on so there could be problems. Please let me know.
* Non-wildcard imports are currently working although no official
modules have been ported over yet. I'd like to do that so that we can
start checking the modules for things like undefined variables but
there's some objections to doing that for 2.1.0. We'll have t ohave a
discussion and make a decision once the ziploading code is basically
feature complete.
* Tracebacks from modules should take a step forward with this code.
Before, by convention, modules had their module_utils imports at the
bottom of the file. This allowed line numbers in tracebacks to match
with the line numbers inside of the module file but tracebacks
generated from module_utils code would have line numbers that didn't
equate to anything on the controller (only in the generated module).
The new ziploader code preserves the separation of files so the line
numbers should be correct.
Next things on my list:
* Ability to specify the compression method for ziploader modules.
python's zipfile module supports no compression and zip-compatible
deflate when python is compiled against the zlib library is [Update:
Done]
* "recursive imports" -- this is to allow imports from module_utils to
trigger importing other module_utils code. Currently this is working
for current ansible modules in a very hacky manner. the current
modules import all of the module_utils code that they use so ziploader
knows to include both modules' code. What I want to achieve is
module_utils code that doesn't depend on anything special in the
module to trigger including its module_utils dependencies.
* Other valid python imports. Current code looks for "from
ansible.module_utils.foo import bar" in order to trigger inclusion of
a module. I want to also include things like import
ansible.module_utils.foo, from ansible.module_utils.foo.bar import
baz, from ansible.module_utils import foo, bar and maybe even from
ansible.module_utils import foo ; from ansible.module_utils import
bar. I'm not yet sure that I'll be able to do this (The big question
is how big a speed hit we take from all of this when combined with
recursive imports) but it would be nice to support the full range of
things that python understands as an import with the ziploader code.
As explained in the PR, this should have feature parity with module
replacer (the current way that modules are constructed for shipment
over the wire). A few things are not 100% compatible but it should be
extremely rare that a module is actually using something that falls
into that category. I've tried to include how to port if the module
does fall into that.
I'll be working on recursive imports and such next. I'm pretty sure
that those will also be ready to merge into the main repo before
feature freeze but wanted to make sure we have a checkpoint to revert
to if something unforeseen crops up.
