[ansible-project] PEP8 compliance in codebase

[Fotis Gimian]

Hey there folks, as a Python developer myself, I was a bit surprised to see
that the Ansible codebase doesn't follow Python PEP8 standards as
per https://www.python.org/dev/peps/pep-0008/

Our team (and many other users)
flake8 https://flake8.readthedocs.org/en/2.3.0/ for validating their code.

With all the work going on with v2.0 to ensure better coding standards and
such, I think it would be a perfect time to consider this also. I'd be
happy to contribute style fixes if you are interested and to implement
flake8 checking for your codebase.

I think that 3rd party modules in the extras repo should also be requested
to comply with PEP8 standards so that the codebase remains somewhat
consistent and standards compliant.

All the best and keep up the great work, I love Ansible!!
Fotis

[Toshio Kuratomi]

Hey, Our CI system runs pep8 and pyflakes separately on every commit
as well and it's been something I've wanted to work on cleaning up
since I discovered it. However, v2.0 has made it harder for me since
time has become more of a premium so your help would be welcome.

Not everyone here is onboard with all of the pep8 style suggestions
(the one I hear most frequently is 80 column lines) so i think the
best thing to do is probably take individual (or a small number) of
flake8 warnings and work on fixing those throughout the code base (can
be one or several pull requests depending on how you best work) and
then move on to the next one. That way you'll know that everyone
here is fine with that particular style change before you spend time
fixing the code for it.

Glancing briefly at current pep8 warnings, I see that four space
indent and mixing tabs and spaces are probably low hanging fruit that
would get the numbers down and make it easier to see what else needs
fixing.

Spaces around operators, brackets, braces, and parenthesis probably
should be submitted separately from indentation but also affects many
files.

Deprecations (like .has_key()) should be acceptable to everyone.

multiple statements on one line is a personal pet peeve of mine but
there's not too many of those.

pyflakes shows somethings that are actual errors that need fixing
(undefined name errors - although many of those are false positives
inside of modules. modules are currently concatenated with snippets of
generic code so some things are defined in the generic code).

If you have some personal favorite style bugs that you'd like to
concentrate on, mention them here so that we can get everyone on board
with the style fix and then you can get busy.

-Toshio

Post by Fotis Gimian
Hey there folks, as a Python developer myself, I was a bit surprised to see
that the Ansible codebase doesn't follow Python PEP8 standards as per
https://www.python.org/dev/peps/pep-0008/
Our team (and many other users) flake8
https://flake8.readthedocs.org/en/2.3.0/ for validating their code.
With all the work going on with v2.0 to ensure better coding standards and
such, I think it would be a perfect time to consider this also. I'd be
happy to contribute style fixes if you are interested and to implement
flake8 checking for your codebase.
I think that 3rd party modules in the extras repo should also be requested
to comply with PEP8 standards so that the codebase remains somewhat
consistent and standards compliant.
All the best and keep up the great work, I love Ansible!!
Fotis
--
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
To view this discussion on the web visit
https://groups.google.com/d/msgid/ansible-project/ce4f568f-05ba-4a83-a937-f8cf63a30ca9%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Brian Coca]

So we have some disagreement here, I would hold off on any style PRs.

I find that style updates w/o having enforcement first will just be a
waste of everyone's time as code will drift from the style as we
accept PRs.
It also breaks attribution and makes it harder for us to look at
why/when was a code made in such a way, specially when judging new
changes against that code.
Aside from forcing many rebases for style reasons, not because bugs
got fixed or features added (which already cause many PRs to need
rebases).

Considering our current backlog and issues before us, I really don't
want to spend time in minor style changes nor reviewing them.

If some code is so bad as to be a great maintenance burden, I would
consider a PR, for the rest I think we can live with extra or missing
spaces around operators.
I think we should look at style enforcement way before we even think
about allowing for style only PRs and only after we have dealt with
the many other issues I consider higher priority than this.

[Toshio Kuratomi]

I think that some style updates that can also have impact on the code
should be added even now. For instance, indentation probably should
be accepted now as non-four space indent and mixing tabs with spaces
may have undesirable effects.

I do see that attribution is lost but on the other hand it's better to
have style changes made in a "style PR" than to have style fixes mixed
in with someone else's actual code changes because they had to change
the style of unrelated sections so they could read and understand it.
That practice hampers both attribution and understanding of the actual
meaningful changes.

Enforcement is great and I'd love for us to have that. But
enforcement is easy if you have a clean base (run checker -- if output
is clean, commit) and hard if you have a base that already fails 2000
tests (run the checker on upstream. Record the errors. Run the
checker on the code with the PR applied. Compare errors and decide
whether any of the errors are new).

-Toshio

Post by Brian Coca
So we have some disagreement here, I would hold off on any style PRs.
I find that style updates w/o having enforcement first will just be a
waste of everyone's time as code will drift from the style as we
accept PRs.
It also breaks attribution and makes it harder for us to look at
why/when was a code made in such a way, specially when judging new
changes against that code.
Aside from forcing many rebases for style reasons, not because bugs
got fixed or features added (which already cause many PRs to need
rebases).
Considering our current backlog and issues before us, I really don't
want to spend time in minor style changes nor reviewing them.
If some code is so bad as to be a great maintenance burden, I would
consider a PR, for the rest I think we can live with extra or missing
spaces around operators.
I think we should look at style enforcement way before we even think
about allowing for style only PRs and only after we have dealt with
the many other issues I consider higher priority than this.
--
Brian Coca
--
You received this message because you are subscribed to the Google Groups "Ansible Project" group.
To view this discussion on the web visit https://groups.google.com/d/msgid/ansible-project/CAJ5XC8%3DzT2xXYFs_c%3DE9FWoh2PzEWM0-JOcuSkMSMSHcQH5N%2Bg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

[Brian Coca]

If it affects how code works, I do not consider it a style change it
is a bugfix.

If they change unrelated sections as part of a fix, I consider that a
style change, if the section was so bad it was unreadable, that is an
acceptable change as I stated above.

For the enforcement I agree, it s a lot easier with a clean base, but
it is useless to cleanup the base and then have a thousand PRs which
can 'dirty' it again. Without having an enforcement in place or to be
put in place right after the style cleanup, it really makes little
sense to me to go down this road.

Aside from the attribution issues, which I don't see a way around, in
many cases the nature of our code makes a lot of the linting and style
checks give out false positives, specifically with modules. One
solution would be to ignore certain types of issues, but that will
also hide the actual real positives.

I'm not against having a style guide and enforcing it, I just see pure
style PRs as near useless at this point, if not counter productive.

Also I think we should be putting our efforts elsewhere right now.

[Brian Coca]

I agree on all points.