black, isort, and doc8 - should we use them in our CI?

[tleonhardt]

black is an extremely popular tool for automatically formatting Python code to match the very opinionated black coding standard.

isort is a tool for automatically sorting Python imports to match a given style.

doc8 is a Sphinx style checker.

mypy is an optional static type checker

Should we incorporate one or more of these tools into our CI process similar to how we have incorporated flake8? Of course, we can tweak the default configuration for each of these tools to make them match our stylistic preferences if we desire.

[xNinjaKittyx]

On top of CI, nowadays its pretty popular to run them with pre-commit

black is great, but I do know there's some people that don't agree with its styling sometimes... (but I prefer it way more than yapf in any scenario).

isort can sometimes be a little annoying to configure if you have any python files that do things on import.

[tleonhardt]

I believe we should use all of these as part of our CI build/test process. We should also use mypy as part of that process, but we might want to wait until after we deprecate support for Python 3.5 for that one.

[xNinjaKittyx]

I was somewhat bored on a weekend, and I did some stuff with pre-commit along with your recommended linters into a branch. Let me know what you think. Most changes were automated by pre-commit, so the more important files to look at would be .travis.yml, setup.cfg, tasks.py, and .pre-commit-config.yaml

https://github.com/python-cmd2/cmd2/tree/ci_improvements

[tleonhardt]

@xNinjaKittyx I really like the basic approach. I'm sorry you were bored, but I'm glad you spent the time doing this ;-) I think we want to tweak the default configurations a little though.

For starters, for black, I recommend we add the setting to ignore forcing double quotes everywhere. Personally I think that is arbitrary and it makes this diff way harder than it should be to look at. I think the correct setting to tweak is:

skip-string-normalization = true

If at a later time we decide we want to follow a stricter adherence to the black defaults, we can remove this.

Also, we need to make sure our settings for flake8 are consistent with those for black. If I run flake8 in the same way our unit tests do using our invoke task, I get the following error:

(cmd2) ➜  cmd2 git:(ci_improvements) inv flake8
0
./setup.py:52:63: E231 missing whitespace after ','
        "test": ["codecov", "coverage", "pytest", "pytest-cov",],
                                                              ^
1     E231 missing whitespace after ','
1

I think that is for a nested setup.py in one of the plugins.

[xNinjaKittyx]

Yeah for E231, it's an unfortunate bug that's in black's 2nd to last release. There's several reports, but here's one for example

psf/black#1465

Regarding why I avoided the latest release is due to this bug
psf/black#1629

I thought I added this to setup.cfg to ignore E231

ignore = E252,W503,E231,E203  # E231 can be removed once black is fixed.

running black twice I think is worse than just temporarily disabling E231 until black has a better release in my opinion. Let me know what you think.

As for the string normalization, I'll do that.

[tleonhardt]

Yeah I'm fine temporarily disabling E231 until black is fixed. If we do that, we need to make sure we do it universally so it is the same for pre-commit, running manually via invoke and running in the unit tests

[tleonhardt]

@kmvanbrunt @anselor Can one or both of you please take a look at the changes on this branch. There are lots of little whitespace changes with the adoption of black. I want to make sure you both think the changes are OK. We can make small tweaks in configuration(s) if that helps.

[xNinjaKittyx]

I had another question...

For invoke flake8, we also invoke flake8 inside ext_test, but the one in the root folder already covers the plugin (ext_test). Would there be a reason why we would want to have 2 separate flake8 runs?

Since setup.cfg is in the root folder, the task inside ext_test will execute with its own parameters. It's fairly easy to fix with a config parameter, but was wondering if we even need this at all?

[tleonhardt]

Good question. I can't remember if there is a good reason for that or not. Not too long ago @anselor moved a core extension inside the main repo and when he did that also ported from using tox to using nox. @anselor Do you have any memory of why we did it this way?

[anselor]

2 things come to mind. One is when I'm working on a component I like being able to run things scoped to that component. So like run validation and testing on just the plug-in without running it on everything. The second is that the plugin was in it's own repo before so it could be a holdover.

I'll try to take a look at the changes tomorrow when I'm in a computer.

[tleonhardt]

@xNinjaKittyx I think we are going to have to refactor some of your work on the ci_improvements branch after PR #1051 merges in. That PR gets completely rid of Travis CI and AppVeyor and replaces them with CI using GitHub Actions Workflows - there is also a Lint and Doc action separate from the main CI workflow.

I envision adding something like a Format action which runs black and isort (and potentially other tools).

I also think that at least to start it would be best to run it on either on push or pull_request like all the other CI workflows. Though, down the road we can consider moving it to run on pre-commit.

[tleonhardt]

@xNinjaKittyx Since we went ahead and made the various CI improvements using GitHub actions, albeit not use pre-commit hooks, I"m thinking of deleting your ci_improvements branch as part of some cleanup. But I'll wait a week or so in case you plan on doing anything more with that branch.

[xNinjaKittyx]

I don't have much knowledge into github actions, maybe i'll check it out sometime in the future. Feel free to delete the branch, thanks for letting me know!