fixing some "broken windows"

[crolfe]

Hi there!

I have been reading through the tests, and noticed a few rough edges that should be fairly easy to clean up. Primarily, these are things that would normally get flagged by an automated linting tool: unused imports, incorrect line lengths, undefined variables etc. I have also come across a few tests that either do not make any assertions, or would not run because they have been redefined in a file (e.g. test_project_attributes in tests/test_meter_library.py)

What I propose is the following:

• add pytest-flake8 as a dev dependency
• resolve the PEP8 issues highlighted by flake8
• add assertions to any tests that are missing assertions (or mark them with a TODO)

This will touch a number of files, so please advise if I should start from a different branch than master to avoid conflicts. I am happy to take this on if you would like to see this addressed.

P.S. in case you were wondering on my inspiration for the title: https://blog.codinghorror.com/the-broken-window-theory/

[philngo]

Hey, thanks for the interest! These suggestions are great, and fit nicely with a couple of other issues (setting up coverage, using tox, etc) on our timeline that will help us avoid code rot and make this library better for developers and users.

Lets have you start from the develop branch, rather than master. We're working on an sprint right now toward v0.4, which will have some breaking API changes - so the three items you've mentioned, i.e. adding pytest-flake8 and resolving PEP8 issues, and adding assertions to tests missing assertions - will be very helpful in making sure that we get things in tip-top shape for v0.4

[crolfe]

Thanks for the quick response. I have started a branch off develop and will let you know if I have any questions as I move along.

By the way, is there any preference for a max line length? 80 columns has been the standard for quite a while, but sometimes 120 can be OK too. At my current workplace we are strict about 80, which can feel a bit restrictive, but I find it forces you to be succinct and seems more readable.

[philngo]

Max line length of 80 is great. I went ahead and out of curiosity installed pytest-flake8 locally yesterday so that I could run py.test --flake8 to see what it would flag, and I'm happy with the default < 80 column limit it imposes. The lines it flagged did seem a bit long, so let's move forward with that.

[crolfe]

I have opened a pull request, but I would like to call attention to a couple of things, which are currently marked with TODOs:

• line 329 in ./eemeter/uploader/api.py: I couldn't tell if we should be doing something with the return value from calling the sync method of consumption_record_uploader. Prior to my changes, this was assigned to a variable, but nothing was being done with it (possible bug?). I removed the variable assignment for now, but will change this if required
• ./tests/test_meter_library.py there are two very similar tests that used to have the same name (i.e. test_project_attributes), but I couldn't tell if they should be kept separate and given better names, or merged into one tests
• tests/test_uploader/test_uploader.py this file has test setup code, but does not contain any assertions. Please advise if there is value in providing assertions here, or if this file can just be removed.