Replace range with enumerate

Archives Ansible Developer
1

In the link https://github.com/ansible/ansible/blob/devel/lib/ansible/cli/__init__.py#L84,

for i in range(0, len(self.arg)):

is bit redundant, when you can just do range(len(self.arg))

But its better (and more Pythonic) to use enumerate:

for index, item in enumerate(self.args):
    arg = item
    if arg in self.VALID_ACTIONS:
        self.action = arg
        del self.args[index]
        break



Also, on another note, consider using for-else to replace:
2

In the link
https://github.com/ansible/ansible/blob/devel/lib/ansible/cli/__init__.py#L84,

    for i in range(0, len(self.arg)):

is bit redundant, when you can just do range(len(self.arg))

But its better (and more Pythonic) to use enumerate:

for index, item in enumerate(self.args):
    arg = item
    if arg in self.VALID_ACTIONS:
        self.action = arg
        del self.args[index]
        break

You are correct, enumerate would be more pythonic. However, do:

for index, arg in enumerate(self.args)
    if arg in self.VALID_ACTIONS:
        [...]

That way you get rid of an assignment statement.

I did have to check whether deleting from the list as we were looping
over enumerate would raise a RuntimeError like it will when you delete
from a dict. It does not so we won't have any problems there.

Breaking out of the loop immediately after deleting protects this code
from the index getting out of sync with the actual position that we
are operating on in the list (A problem that would have affected the
range() based code as well).

So if you want to pull request this, that is fine.

Also, on another note, consider using for-else to replace:

if not self.action:
    raise AnsibleOptionsError("Missing required action")

with this:

for:

    ...

else:

    raise AnsibleOptionsError("Missing required action")

Since for-else is not present in other languages I like to be sparing
in its use. If I do use it I always add a comment that explains what
it is (otherwise the next programmer to look at the code is likely to
think the code is supposed to go with an if statement within the loop
and reindent it. So if you make this change in your PR, I'd suggest:

else: # This is a for-else: http://bit.ly/1ElPkyg
    raise AnsibleOptionsError("Missing required action")

-Toshio