Removed two unused imports in two tests.

[willynilly]

I removed the unused imports mentioned in the following issue:

#208

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (master@e9b82f0). Click here to learn what that means.

Additional details and impacted files

@@            Coverage Diff            @@
##             master     #209   +/-   ##
=========================================
  Coverage          ?   95.48%           
=========================================
  Files             ?       17           
  Lines             ?     2083           
  Branches          ?        0           
=========================================
  Hits              ?     1989           
  Misses            ?       94           
  Partials          ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[andrewgsavage]

this is a lot more than just moving two tests as you've changed line spacing and whitespace throughout those files. I assume you've run this through a linter. We'd like to get a linter running in the CI to give formatting throughout. I think changes to formatting should be done after a linter is set up in CI.

[newville]

I agree with @andrewgsavage. Removing the imports is fine, but that should change 2 lines of code.

[willynilly]

i am curious. what is the problem with changing line spaces before you add a linter to the build process? my changes passed all the tests.

[jagerber48]

i am curious. what is the problem with changing line spaces before you add a linter to the build process? my changes passed all the tests.

While your efforts to clean up the code base are much appreciated, we should keep each PR as focused as possible so it's more easier for everyone to follow what changes were made where and when. This PR should handle the imports like its title suggests, but the job of whitespace cleanup should be handled in a dedicated PR. As has been said, there's some likelihood we'll have automated CI in the future which will include formatting. It will be best to bundle all the "formatting" changes into a single PR around that time.

I understand the temptation to "clean up" things like this as you go. But,, I've come to learn, it causes the repo documentation to get muddled some, and this matters more for a heavily used public repo that a team of people work on. So I think it's best practice to avoid "sneaking" general cleanup into PRs dedicated for something else.

Furthermore "passing tests" is not the only criteria for PR acceptance.

Another argument against sneaking formatting changes in is this. You might prefer to format things one way, but someone else might prefer another way. You could then imagine a PR war happening in the repo where people have different opinions about formatting. This is one of the advantages of setting up CI to handle formatting. Then we agree, collectively, on a set of documented opinions about formatting, and those opinions get implemented automatically.

[willynilly]

Well, I appreciate the code review and explanations. I have resubmitted another pull request with the minor changes requested. For documentation purposes, the previous reformatting that was rejected was using https://pypi.org/project/autopep8/.
I did not mean to "sneak" in any PEP8 formatting.