General code cleanup

[anson-vandoren]

Just a question/discussion, not an issue:

Do you have any strong opinions about overall code style/format? Specifically, most other projects I work own or contribute to use linters, code formatters, static type checkers, etc. in order to help maintain consistency between contributors and also to pick up on code smell early.

If you really prefer to keep it the way it is, no problem and I'll stick with the style you already have going on.

But if you're not averse to it, I'd be happy to do the work to standardize the project. My preferences would be:

• pylinter (code inspection)
• black (code formatting - note that it enforces 88 column lines instead of the 80 that most of the code currently is at)
• mypy (static type hinting)

I think in the longer term (and I intend to keep using this library for quite a while) this would lead to better code quality, easier contributions from others, and enhance readability.

Relatedly, I would like to start writing unit tests. That's less project-wide impact, but just the same, do you have any preference on testing framework? Again, I'd be happy to get things going in the right direction if you agree, but don't want to step on toes here 😄

Let me know what you think, and thanks again for a great project!

Regards,
Anson

[anson-vandoren]

Disregard the section about unit tests. I had somehow missed all the tests you'd already written. Based on those, I would assume you'd like to stick with unittest as a framework?

[anson-vandoren]

And one additional thing I was just thinking of (based on one of the errors in a PR from last night):

Have you considered setting up something like Travis and Coveralls to hook into PRs to check for build failures and decreases in test coverage prior to accepting a PR?

Obviously not critical (and especially with few contributors right now), but both are free for open source projects, and it does give a little more confidence on accepting a PR.

Just some more to think about :)

[probstj]

Thank you very much for your suggestions. I have looked into it all, and I like all of it very much! I tried following PEP 8 and PEP 257 as much as possible, so totally agree in using "black" to handle it automatically.

With "mypy", will static type hinting be handled/added automatically, or do we need to add hinting to the code ourselves? In any case, I prefer the way it is added which is python2-compatible (I am not very used to the py3.6+ type hinting code yet, and find the hinting by comments less intrusive). Also, the code is still python2-compatible and I would rather like to keep it like that (though that's not a very strong opinion of mine, so I am open to discussions if you have any reason to drop py2-compatibility, but I haven't seen any reasons to drop it so far).

Yes, I have included a very few tests using unittest, but you are completely right, we need to try to cover all the code with unit tests so have to add a lot more. I like using stuff that's already included in Python, keeping the dependencies as little as possible, that's why I decided to use unittest. But if you have any strong reason/opinion to use something else, I am open to suggestions... :)

Also, Travis CI and Coveralls (which Python-lib do you prefer?) are great and I would definitely like to add it to this project.

[anson-vandoren]

Thanks for the response!

• For mypy, we'd need to manually add static type hints.
  • It can be done incrementally, and so no need to make any changes right off the bat.
  • I wanted to start adding the hints as I was refactoring/adding/fixing other things, but wanted to make sure it was OK first. Python2-compatible hints are fine with me.
  • Unfortunately, we're going to have a lot of mypy reported errors based on current dependencies for right now (pandas, babel, weasyprint all missing type hints)
• In terms of overall python2 compatibility, I don't have any objections. There are a couple of things I've had to do a slightly different way on this project than I would have at first to maintain compatibility, but it's not a big deal, and shouldn't be going forward either. I guess once the 'official' sunset date in 2020 happens, we can discuss again if there's any reason to drop support
• Unittest is fine for tests. I usually use pytest for my own projects, but not for any reason other than at this point it's what I'm most familiar with.
• I've set up Travis on other repos, and it's pretty straightforward to do. I've not personally setup Coveralls before, but seen it used on some other projects I've contributed to, and the documentation makes setup look pretty straightforward. We should be able to run Travis against as many variants of python2/3 as you'd like, and I don't think Coveralls should care which version (coverage should be the same regardless, since it's running the same tests).
• regarding pylint, the biggest complaints it's giving me right now are:
  • docstrings for classes and modules
  • non-conforming variable naming
  • various formatting discrepancies
  • As with mypy, I think these are things we can mostly just change in conjunction with other changes/refactorings/fixes

I'll start using black, pylint, and mypy as basis for future commits. If you want any help setting up Travis or Coveralls, let me know.

Thanks again,
Anson