Biopython: Enforce flake8 E127 and E128 on Tests/

[peterjc]

This is a continuation of #1515 which allowed us to enforce the flake8 style checks E127 and E128 on Bio (and this was already enforced on BioSQL, Scripts and Doc/examples).

This is part of an ongoing effort to more closely follow the PEP8 style guide, see https://www.python.org/dev/peps/pep-0008/

I'm marking this as a good first issue for a potential new contributor - it does not require any prior knowledge of Biopython's internals, although familiarity with Python syntax is needed.

1. Read https://github.com/biopython/biopython/blob/master/CONTRIBUTING.rst and setup flake8
2. Setup a git clone of Biopython with a fork on Biopython, see e.g. https://biopython.org/wiki/GitUsage
3. Make a new branch, e.g. git checkout -b flake8_tests_e127
4. Edit Tests/.flake8 to remove E127 and/or E128 from the ignore list
5. cd Tests/
6. flake8 --count .
7. Fix the associated indentation in the Tests/*.py files, repeating step 6 as needed.
8. Commit your changes to your branch
9. Push your branch to your Biopython fork
10. On the GitHub website, make a pull request referencing this issue.

Note: Right now there are about 130 cases of E127, and 670 for E128 - so you could certainly do this in two batches.

[nimne]

I'll take a crack at this. It'll be a good chance to get familiar with the testing code!

[nimne]

Couple questions as I'm working through this.

It looks like PEP 257 recommends that single line docstrings look like this:

"""Test repair of a single ambiguous position in a genome."""

rather than:

"""Test repair of a single ambiguous position in a genome.
"""

Should I fix these along with this issue, or leave this for a separate pull request?

Also, there are a few examples of tests that look like this

    def test_single_repair(self):
        """Test repair of a single ambiguous position in a genome.
        """
        repairer = AmbiguousRepair(self.ambig_info, 2)

        for repair_attempt in range(5):
            new_org = repairer.repair(self.organism)
            new_genome_seq = new_org.genome.toseq()

            assert new_genome_seq.count("*") == 2, \
                   "Did not repair genome, got %s" % str(new_genome_seq)

I can fix the E127 error that is raised by the assert line by changing it to a single indent after the newline, but that may not be the best solution. If I'm reading through the unittest documentation correctly, would it be appropriate to change that assert statement to this?

            self.assertEqual(new_genome_seq.count("*"), 2,
                             "Did not repair genome, got %s" % str(new_genome_seq))

I figure I can either I can change the assert statements to the unittest assert*() methods or I can just address the the E127 / E128 errors. My preference is to change all of the assert statements to the assert*() methods now as I'm digging through this, but if that makes sense as multiple pull requests I can do that too.

From the unittest documentation: "To make migrating existing test suites easier, unittest supports tests raising AssertionError to indicate test failure. However, it is recommended that you use the explicit TestCase.fail*() and TestCase.assert*() methods instead, as future versions of unittest may treat AssertionError differently."

[peterjc]

Yes, PEP8 does recommend that for the one-line docstrings, and we enforce this on Bio/ etc already. A second pull request fixing that on Tests/ and updating Tests/.flake8 to enforce it would be great.

[peterjc]

Also yes, where the test files are using the unittest framework we should be using self.assertTrue(statement, message) or similar over assert statement, message - doing that as part of the E127 pull request makes perfect sense to me. This syntax with brackets is also more elegant for multi-line statements.

And yes, try to use the more specific self.assert* methods as appropriate - almost all of them are available on Python 2.7 onwards.

[peterjc]

Closed via #1780, thank you @nimne!