coding style

[MattHJensen]

At some point we need to go through our code and make it conform to a set of style standards. PEP 8 seems like a good starting point. Is it possible to make pylint run automatically as part of our commit suite (along with Travis-CI?)

[iliakur]

I didn't want to drop this on everyone till we had some functional code, but since it's come up, yes there are some automatic tools out there to check PEP8. However, we should keep in mind that these are conventions, so I would personally be more comfortable with some human inspection of the code to resolve any ambiguous situations and maybe bend the rules where necessary. As the guide itself states, foolish consistency is the hobgoblin of small minds. My concern is that automated linting will enforce precisely that kind of consistency.

[MattHJensen]

sounds good.

[talumbau]

I have done some projects where something like pylint was used as a test, so "passing all tests" meant "everything is PEP8." I've also done projects without that. Even w/o the automated checkers, I will usually try to keep to these standards:

• max of 80 characters
• 4 space indents (no tabs)
• no lines with blank spaces at the end

That's a good list for me. YMMV.

[feenberg]

Those are good rules - especially "no tabs". I wonder if it is worthwhile enforcing style rules before we remove the numpy syntax.

dan

[iliakur]

I've actually had to remove the tabs in my local copy of the translation.py because my editor is set up to add use spaces, but Sameer's code had tabs and the Python interpreter can't handle a mix of both. Getting rid of tabs and spaces can be done before we switch to "vanilla" Python syntax.

[talumbau]

With the merge of #40, translation.py should not have any tabs in it. Let me know if that is not true for you.

[talumbau]

We have not strictly enforced PEP-8, but have eliminated tabs in the code. If there is a desire to enforce PEP-8, we can run flake8 on the source. Closing.

[MattHJensen]

Based on discussion with @martinholmer, I believe it would be useful to begin enforcing some style rules more systematically than we do now. From early discussion, the three rules that have been requested are

• max of 80 characters
• 4 space indents (no tabs)
• no lines with blank spaces at the end

I'm hoping to discuss:

1. Is this the right set of rules?
2. How should we enforce rules?

[zrisher]

In my experience, those are standard to every major open source project on github (besides indent, where 2 spaces is the norm in other languages, but of course in Python 4 spaces is expected).

While automated linting tools can be a pain in the bum sometimes, I believe most projects with large contributor bases have found them to be a significant boon to productivity. While some things will likely be more elegant if the rules are broken slightly, its hard to imagine style enforcement really stopping a useful PR.

[martinholmer]

Based on my experience developing the OSPC ACA-refundable-tax-credit calculator in Python,
I think these three style rules are way too limited. As you may know, Python is the official scripting
language at Google, and they have a small team who has worked for years developing the
Google Python Style Guide.
I'm not advocating that this project try to follow that Style Guide, but it is important to note that
the very first item in the Style Guide is an admonition to use pylint to check your code.

Here is what the Google Python Style Guide says about this:

Lint

Run pylint over your code.

Definition:
pylint is a tool for finding bugs and style problems in Python source code.  
It finds problems that are typically caught by a compiler for less dynamic 
languages like C and C++.  Because of the dynamic nature of Python, 
some warnings may be incorrect; however, spurious warnings should 
be fairly infrequent.

Pros:
Catches easy-to-miss errors like typos, using-vars-before-assignment, etc.

Cons:
pylint isn't perfect.  To take advantage of it, we'll need to sometimes: 
a) Write around it,  b) Suppress its warnings, or c) Improve it.

Decision:
Make sure you run pylint on your code.  Suppress warnings if they are
inappropriate so that other issues are not hidden.

To suppress warnings, you can set a line-level comment:

dict = 'something awful'  # Bad Idea... pylint: disable=redefined-builtin
pylint warnings are each identified by a alphanumeric code (C0112) and 
a symbolic name (empty-docstring).  Prefer the symbolic names in new 
code or when updating existing code.

If the reason for the suppression is not clear from the symbolic name,
add an explanation.

Suppressing in this way has the advantage that we can easily search
for suppressions and revisit them.

You can get a list of pylint warnings by doing pylint --list-msgs.
To get more information on a particular message, use pylint --help-msg=C6409.

Prefer pylint: disable to the deprecated older form pylint: disable-msg.

In addition to specifc warnings, pylint produces a score for the checked
code that typically ranges from zero to ten, with ten being achieved if there are
no warnings. The existing OSPC code I have checked with pylint typically
has many dozens of warnings and a negative score. Such a situation makes
it impossible for a new contributor to use pylint to help check the correctness
of the new code being contributed. This is a major problem for our project. If this
poor-code-style problem is not addressed, I fear that there will be few who will be
able to contribute effectively to the project. That would be a sad outcome.

[rickecon]

I think @talumbau and @martinholmer have the right idea. Make pylint part of the test suite. You can also install a package in your text editor that tells you if your code satisfies PEP8 guidelines.

[talumbau]

I have actually found that too much adherence to a particular standard can discourage contribution, but I think there is plenty of room in the middle, and in particular, since we have no enforcement right now, we are far from that point.

I am actually more partial to pyflakes, and flake8 in particular:
https://pypi.python.org/pypi/flake8
It's less opinionated than pylint, and seems to "get in the way" less frequently. I think that successful projects that use pylint end up having an agreed upon set of 'turn off all of these things' so that warnings are manageable. If pylint is chosen, we should create an issue to hash out the agreed upon set of warnings.

In either case (either "flake8 'out of the box'", or "pylint + some set of things to turn off"), I would advocate that we add either the pytest-flakes plug-in or the pytest-pylint plug-in (depending on which is chosen). Then, we would modify Travis-CI to run pytest with these checks. That way, once we determine the code is in the style/format we deem to be good, it is impossible to add code that violates the standards.

A modest approach right now might be to start with just basic PEP8 enforcement. We could enforce that with pytest and then move to stricter standards after we are first fully PEP8 compliant.

[martinholmer]

TJ @talumbau said:

A modest approach right now might be to start with just basic PEP8 enforcement.
We could enforce that with pytest and then move to stricter standards after we are
first fully PEP8 compliant.

This seems like a reasonable approach: there would be a pytest-enforced minimal
standard (that would get more comprehensive over time) and people could use
whatever style-checker (for example, pylint) they want as they develop code on
their local computer.

[feenberg]

On Tue, 28 Jul 2015, zrisher wrote:

In my experience, those are standard to every major open source project on github
(besides indent, where 2 spaces is the norm in other languages, but of course in
Python 4 spaces is expected).

While automated linting tools can be a pain in the bum sometimes, I believe most
projects with large contributor bases have found them to be a significant boon to
productivity. While some things will likely be more elegant if the rules are broken
slightly, its hard to imagine style enforcement really stopping a useful PR.

I am not familiar with pylint, but I find it very helpful to make the
changes to source code necessary to compile fortran without generating any
warnings. Even if the complaint is unjustified, it is usually easy to
satisfy the compiler with small changes and it makes debugging much easier
to have no other warnings.

dan

—
Reply to this email directly or view it on
GitHub.[AHvQVVYbVsAjKncmmz37SN1YK1RgIOvoks5oh93igaJpZM4Cjldb.gif]

[MattHJensen]

The incremental approach seems wise to me as well: start with PEP8, and then add more after we are PEP8 compliant.

It also seems important to accommodate people who would like to use some other style-checker, such as pylint in the interim.

For both, getting to PEP8 compliance and accommodating pylinters in the meantime, we'll want to continue to be careful to separate out style changes from substance changes, preferable into separate PRs, but certainly into separate commits. This raises another question:

How do we separate style from substance?

Option one:

Procedure: I first make style changes to the section of code I am going to be working on and open a PR. Then I check out a branch on top of that one and make the substantive changes to the same section. Open another PR.

Upside: I get to zero warnings for the relevant section for both pylint and PEP8 before I make substantive changes, and so I know that any warnings that pop up after the substantive changes indicate problems that I introduced.

Downside: My substantive PR now relies on the style PR to get merged first. This is probably fine if there were only PEP8 changes made in the first, 'style' PR, but what if I made changes in the first PR to get pylint warnings to go away, but those warnings were ones that the we haven't had an opportunity to discuss whether we care about yet? Now we are having that discussion on an ad hoc basis about this style PR and holding up the substantive PR

Option two:

Procedure: I first open a PR with PEP8 changes, then I open a PR with substantive changes on top of that, then I open a third PR with pylint changes on top of the substantive changes PR.

Upside: Gives the community more time to discuss the pylint-inspired changes without holding up the 'substance' PR.

Downside: this eliminates our ability to use pylint to warn of any new problems that we introduce with the substantive change.

Are there other options? Am I making this too complicated?

[MattHJensen]

Option three:

Procedure: We put substantive PRs on hold for a little while and make a big push on PEP8 and get that out of the way. Then we see how angry pylint remains, and based on that decide whether to proceed with a "style first, substance second" or "substance first, style second" PR strategy.

[martinholmer]

Dan @feenberg said:

I am not familiar with pylint, but I find it very helpful to make
the changes to source code necessary to compile fortran
without generating any warnings. Even if the complaint is
unjustified, it is usually easy to satisfy the compiler with
small changes and it makes debugging much easier to have
no other warnings.

I agree 100%, probably because I'm also coming to this project
after years of experience writing source code that is compiled.

I want to contribute to the Tax-Calculator project, but as an
inexperienced Python programmer, I want to be able to check
my revisions to the source code with some kind of tool like pylint.
But it is almost impossilble to do this now because the existing code
generates so many warnings that I find it difficult to figure out which
of the dozens of warnings are being generated by my code changes.

I think any contributor would like the situation you describe above.

[martinholmer]

@MattHJensen said:

Are there other options [besides option 1 and option 2]?
Am I making this too complicated?

This is great you are giving this issue some thought, thanks @MattHJensen.

My reaction is that yes, options 1 and options 2 are too complicated.

Why not do something like your option 3, where you assign a taxcalc
class to a team member asking that person to leave the substantive
logic and API of the class unchanged, but eliminate pylint warnings
from that class and from the tests for that class?

Most pylint warnings have been eliminated in the Parameters class
in pull request #321, but there is also an update method bug fix. Going
forward, I think you are right to call for a separation in pull requests that
make stylistic changes (to eliminate warnings) from pull requests that
fix a bug, refactor the API, or add a new capability. But once warnings
have been eliminated from the existing code, there will not be any need
for the complexity of your first two options.

[talumbau]

I don't agree that the coding standard for this project should be 'no output from default pylint'. If we choose the pylint tool, we will need a separate issue to discuss which warnings we turn off/on. I would propose an 'opt-in' strategy rather than an 'opt-out' for the chosen set of warnings. PEP8 is fairly easy to enforce and would also be easy to do as single PR. My suggestion is that we execute on the plan 'make everything PEP8, enforce PEP8 in the tests' and then reassess when we are standing atop that hill.

[martinholmer]

TJ @talumbau said:

I don't agree that the coding standard for this project should be 'no output from default pylint'.

Who are you quoting here? I've reviewed this whole discussion and don't see anybody
advocating that position. So, why do you have to disagree with a view that is not being
expressed in the conversation?

[MattHJensen]

Synthesizing my, @martinholmer, and @talumbau's recent suggestions, here's a proposed plan.

• I will open an issue and a PR that brings the code to PEP8 compliance.
• After that, @martinholmer will open a separate issue that lists all of the classes of warnings that pylint generates and opens a discussion on which to keep or whether to use some alternative to pylint.
• After the PR from step 1 is merged, we can get an idea on what kind of warnings pylint (and alternatives) generate about our current code base, and we will be in a better position to have a productive discussion in the issue that Martin opened with step 2 (above).

[martinholmer]

@MattHJensen said:

Synthesizing my, @martinholmer, and @talumbau's recent suggestions, here's a proposed plan.

I will open an issue and a PR that brings the code to PEP8 compliance.

After that, @martinholmer will open a separate issue that lists all of the classes of warnings that pylint generates and opens a discussion on which to keep or whether to use some alternative to pylint.

After the PR from step 1 is merged, we can get an idea on what kind of warnings pylint (and alternatives) generate about our current code base, and we will be in a better position to have a productive discussion in the issue that Martin opened with step 2 (above).

This sounds like a reasonable plan to me.

[MattHJensen]

@martinholmer, do you think this issue should be closed?

[martinholmer]

@MattHJensen asked:

@martinholmer, do you think this issue should be closed?

Yes.
I think enforcing PEP8 guidelines (as we have been doing for many months), and allowing optional use of pylint, is a good solution for this project.

@feenberg @talumbau @Amy-Xu @GoFroggyRun @zrisher @codykallen

[MattHJensen]

I think enforcing PEP8 guidelines (as we have been doing for many months), and allowing optional use of pylint, is a good solution for this project.

Thanks @martinholmer. That makes sense to me too. Closing.

[OutsourcedGuru]

I know this is from way-back-when but I just cringe at conversations like this. It began sanely enough for the idea that a little commonality be found in the code. It was warned that the concept of linting for the sake of linting is the work of small minds. Fast-forward and we see that enforcing PEP8 sounds like what's needed. 🤦

Every time someone adds a PEP8 github build criteria to a project God kills a kitten.

[hdoupe]

@OutsourcedGuru PEP8 grinds my gears, too. I like Black because it takes care of all of the silly formatting rules like line length for you. This lets me focus on writing code and not constantly fixing my code formatting. That being said, PEP8 has been helpful for enforcing some readability rules on the project, and we'll most likely keep using it for this purpose.