pep8-ex-e402 compliance

[MattHJensen]

Deals with #328.
Replaces #329.

This is ready for review by @talumbau and @martinholmer.

The path forward would be:

1. Merge this
2. Add pep8-ex-e402 to the test suite.
3. On a more-relaxed timeline:
   • Figure out how to deal with e402
   • Deal with many merge conflicts in outstanding PRs as well as the new requirement to be complaint with pep8.

[martinholmer]

This seems like a sensible approach to move forward towards PEP8 compliant code.

[MattHJensen]

Thanks @martinholmer.

I've been waffling on whether we should have a longer line length standard, perhaps 90. As we add policy reforms options in functions.py, we will need some English-descriptive-named variables that will be significantly longer than the e-codes. The number of odd continuations will only grow over time. we necessarily need to settle this question before this PR is merged, but I bring it up because it would be nice to have feedback of whether there are any other pep8 standards (like line length at 79) that might lead to harder-to-read and write code in the long run. I'll note that a significant portion of the Python standard library is not pep8 compliant, so, while I whole heartedly agree that the project is at a point where we need some standards, those standards do not necessarily have to exactly match pep8.

[martinholmer]

For me the line-length-less-than-80-characters requirement in PEP8 is the
single most important requirement for readable Python code. Without it,
the code is difficult to read in Emacs and is not very readable when printed
out on paper.

I don't understand why using the built-in Python string concatenation
capability is not a reasonable approach to specifying long strings in
in Python code. Can you provide a concrete example of what you
are worrying about?

[feenberg]

On Sun, 9 Aug 2015, Matt Jensen wrote:

Thanks @martinholmer.

I've been waffling on whether we should have a longer line length standard,
perhaps 90. As we add policy reforms options in functions.py, we will need

I agree that continued lines are hard to understand, and I often add
variables to break up expressions that would force a continuation on me.
Typically my editor is set for 94 characters on a line, and that is
readable and can be printed. Anything above 132 is unprintable and
unreadable.

dan

[talumbau]

@MattHJensen are you running as follows for this PR:

cd taxcalc
pep8 --ignore=E402 .

If yes, I get additional PEP8 errors on your branch. The only way I get no errors with this branch is:

pep8 --ignore=E402,E226,E241,E121 .

Am I doing something incorrectly?

[talumbau]

hmm.. I just created this PR against your branch Matt: https://github.com/MattHJensen/taxcalc/pull/3
It doesn't look like Travis CI will run with a PR set up as Matt's fork <- T.J's fork. Let me create it against OSPC taxcalc and see if I can get travis CI to run.

[talumbau]

OK, cool #339 passes Travis, but I'm not sure if the intention of this PR is just to have E402 be the only ignored PEP8 violation. If so, additional work must be done because, as far as I can see, there are still violations of E226, E241, and E121.

[talumbau]

Also, one consequence of 'Travis CI enforcement of (some form of) PEP8 compliance': all will have to run the PEP8 tests with:

py.test --pep8

or

py.test --pep8 -m pep8

to just run the PEP8 tests. If you just run py.test and then push with no failures, Travis CI might fail if you have PEP8 violations, which could be somewhat confusing for newcomers.

[martinholmer]

Are not E226, E24, and E121, worth enforcing? What is the rationale for excluding these checks?
I understand that we haven't yet figured out how to do relative imports, so we are not enforcing E402.
But what is the rationale for dropping enforcement of these other three PEP8 rules?

[talumbau]

@martinholmer the additional exceptions in my commit were just done because that is what was needed to pass the tests. My currently outstanding question to Matt is: do you intend that this PR brings the code in to PEP8 compliance except for E402? The information I have that is relevant to that question is as follows: this PR only brings the code into PEP8 compliance if you ignore E402, E226, E241, and E121 (as far as I can tell). I think that's a good thing to know. It seems like we have some choices on how to proceed:

• add more commits to this branch so that we have PEP8 compliance minus E402
• except some or all of the E226, E241, and E121 errors.
• demonstrate that I ran the PEP8 stuff in a different way from Matt and I made a mistake somewhere, which would then go into the documentation of how we run the PEP8 checks.

[martinholmer]

TJ @talumbau, we have too many pull requests with comment streams in both!
Sorry I missed your comment just above.

I agree completely with your analysis of the situation.
I think the first of your three options is the most sensible by far:

add more commits to this branch so that we have PEP8 compliance minus E402

Otherwise, we need a discussion about what the rationale for excluding other PEP8
rules (in addition to E402).

[MattHJensen]

@talumbau and @martinholmer, my intent is to bring this branch into PEP8 compliance, only excluding E402. Interestingly, when I run pep8 taxcalc from ~/tax-calculator/ (or even when I run pep8 tax-calculator from ~/), then I only get E402 errors. Additionally, a pep8 checker plugin for my editor finds no other errors.

But I do see the other errors when I follow @talumbau's suggestion and run

cd taxcalc
pep8 --ignore=E402 .

Visual inspection clearly indicates that the non E402 errors are indeed present, so I will add E402 ignore to my Sublime linter and add more commits to this branch. Coming today.

[MattHJensen]

@talumbau, could you please review and merge?
Let me know if you would like me to merge https://github.com/MattHJensen/taxcalc/pull/3 first (although that would need to be updated to only exclude e402 now)

Diagnostics tables verify that this PR doesn't change the results.

Branch:

Master:

[talumbau]

Yes I think it would make sense to merge in https://github.com/MattHJensen/taxcalc/pull/3 for this PR, then change the setup.cfg to say

[pytest]
pep8ignore = E402

instead of the current list. Then we would know that TravisCI agrees with the statement "this set of code is PEP8 compliant minus E402" before merging. I can do this later today if you would like.

[MattHJensen]

Alright @talumbau. just merged and updated setup.cfg.

[talumbau]

enthusiastic 👍 thanks for doing all the heavy lifting @MattHJensen

[MattHJensen]

@talumbau, could you merge if you're satisfied? PEP8 isn't a bad starting point, even if we decide to make some deviations later on.