Decide on standard configuration for Python

[klauer]

Current test one is:

$ cat .pre-commit-config.yaml

# See https://pre-commit.com for more information
# See https://pre-commit.com/hooks.html for more hooks
repos:
-   repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v2.5.0
    hooks:
    -   id: trailing-whitespace
    -   id: end-of-file-fixer

-   repo: https://gitlab.com/pycqa/flake8
    rev: 3.7.9
    hooks:
    -   id: flake8

Add:

• [x] Import sorting via isort?

[hhslepicka]

Shopped a couple for suggestion:

• [x] check-case-conflict - Check for files that would conflict in case-insensitive filesystems
• [x] check-json - This hook checks json files for parseable syntax.
• [x] check-merge-conflict - Check for files that contain merge conflict strings.
• [x] check-symlinks - Checks for symlinks which do not point to anything.
• [x] check-xml - This hook checks xml files for parseable syntax.
• [x] check-yaml - This hook checks yaml files for parseable syntax.
• [x] debug-statements - Check for debugger imports and py37+ breakpoint() calls in python source.
• [x] trailing-whitespace - This hook trims trailing whitespace.

[ZryletTC]

I've tested implementation of all of @hhslepicka's suggestions in my pcdsdevices repo. Only issue they've had is the YAML checker doesn't like conda-recipe/meta.yaml:

found character that cannot start any token in "conda-recipe/meta.yaml", line 1, column 2

I've found this line in conda-build documentation and we've used it for years so it definitely works. I guess it just defies regular YAML rules? Should there be exception for this file or do any of you know how to do it the right way?

[klauer]

https://github.com/pcdshub/pcdsdevices/blob/master/conda-recipe/meta.yaml#L1

I think conda-build uses some pre-processing of the yaml file. Wonder if there are any meta.yaml checkers out there like what conda-forge uses to lint?

@ZryletTC can you ignore certain files for that check-yaml step?

Edit: exclude is an example here: https://pre-commit.com/#regular-expressions

[ZryletTC]

Using isort definitely makes it look nice but wow it made a lot of changes. In regular files, all changes were good but in some of the configuration files, I'm not knowledgeable enough to tell if those placements were intentional.

For example, did we want to leave this import down there for some reason? https://github.com/pcdshub/pcdsdevices/blob/b82619696e20740557ba51d1fa9e803c4ba8ab4e/docs/source/conf.py#L73 Because the imports were down in the code, isort got confused with the comments, thinking they were about the imports instead of the code below them and tried to move the comments up to the import section.

And here, did we put the larger imports at the end for sake of speed or other reason? https://github.com/pcdshub/pcdsdevices/blob/b82619696e20740557ba51d1fa9e803c4ba8ab4e/pcdsdevices/areadetector/__init__.py#L6

I like isort but we have to be careful about accepting the changes it makes.

[ZLLentz]

The first file is fine to move the import to the top, I wasn't taking care when editing it because it was just the docs configuration file.

The second file was just done with haste and not checked very hard in review, note the commit is from three years ago from Abdullah, "Committing before a meeting". I don't think this file even passes flake8, note how we lazily exclude this directory in the config: https://github.com/pcdshub/pcdsdevices/blob/b82619696e20740557ba51d1fa9e803c4ba8ab4e/setup.cfg#L8

There are a few cases where import order matters (stares in matplotlib), but those should be pretty rare because it makes life difficult. The only other times where odd import placements are functional is when they are inside functions, this typically indicates an optional dependency.

[klauer]

Yeah, I think those isort results are still good.

On a per-repo basis, we can exclude files if really necessary.

[hhslepicka]

Sorry if it is a dumb question but does isort ignore the imports inside of functions?

[klauer]

Looks like it ignores them, likely assuming they're in that function in that order for a good reason:

$ cat test.py
import b
import c
import a

def bcd():
    import a
    import b
    import c
    import _a
(pcds) klauer-osx:Repos klauer$ isort - < test.py
import a
import b
import c

def bcd():
    import a
    import b
    import c
    import _a

[hhslepicka]

Awesome! 👍

[klauer]

Let's add:

https://github.com/pre-commit/pre-commit-hooks

• [x] check-ast: Simply check whether files parse as valid python.

Consider:

• [x] no-commit-to-branch for master (Protect specific branches from direct checkins.); to push people to make PRs

[ZryletTC]

Added check-ast and no-commit-to-branch I think protecting the master is good and if someone really needs to, they can bypass the check.