Details
-
Type:
RFC
-
Status: Implemented
-
Resolution: Done
-
Component/s: DM
-
Labels:None
Description
I've been using black, isort, and mypy in my own development as much as possible recently (i.e. mostly just on new files), and we would really benefit from always using them consistently across our predominantly middleware packages:
- daf_butler
- pipe_base
- ctrl_mpexec
- obs_base
I'm hesitating to include ctrl_bps here, because I very rarely work on it directly and I don't know if its somewhat distinct set of developers want it to be included here.
To be precise:
- I am proposing that we run mypy in PR CI in all of these packages (just as we do already in daf_butler). Many of the other packages already have some type annotations, and just like any other aspect of code that isn't tested, they're very often incorrect. Running mypy will fix that and make sure it stays fixed, without requiring type annotations in new code, even in those packages .
- I am proposing that we run black and isort once up-front on all files in these packages, and then at least check in PR CI that running black and isort would be no-ops; I'm open to the idea of running them in git commit hooks, too, but can't personally make a good case for or against.
I won't go into great detail about the general benefits of these tools - they're widely discussed on the broader internet. I'm proposing we adopt them now because:
- they now have broad adoption in the Python community, and despite some lingering "beta" labeling, they are in use in production code in organizations much larger than ours;
- the packages I am proposing using them in are primarily developed by a subset of the DM team that I think would be comfortable with this move;
- they are very much the kind of tools that need package-wide buy-in from all contributing developers; the utility of using them on an individual opt-in basis is much lower (and in some cases may even be negative).
I do also think Science Pipelines development would benefit from using these tools more, but I'd prefer to leave that for another RFC, especially after implementing this one has helped us smooth out any rough edges in the procedure. I will say that I now very much regret not weighing in strongly in favor of RFC-649 when the question of black was brought up previously (I was ambivalent, having not tried it before). And I think conditions have changed enough for us to consider it again.
Attachments
Issue Links
Activity
Response has been pretty thoroughly positive (some concerns were raised, but seem to have been addressed), and I've created implementation tickets for both MyPy and black formatting with checking via GitHub action. I am not creating a ticket to add a pre-commit hook for black right now (there are instructions at https://black.readthedocs.io/en/stable/integrations/source_version_control.html) unless someone asks for it; I'm personally inclined to lean on in-editor formatting and don't think I'll need a pre-commit hook in my workflow.
I've now got the formatting part of this RFC implemented on DM-32437 branches, and I figured this was a good place to document the wrinkles that came up:
- isort will recursively run on all files containing Python code in each directory it is given, regardless of suffix, while black will only look at .py files. That means bin.src directories (whose files often don't have .py extensions) should to be formatted with black bin.src/*; isort bin.src.
- isort's reordering of __init__.py imports have revealed a moderately common pattern that actually involves circular imports: doing from . import <lifted-symbol>. Unlike from .<sibling> import <symbol>, the former re-imports __init__.py, and should be avoided.
- black and flake8 differ on spaces around colons, and black claims that it's the one that's PEP8 compliant, so I've disabled E203 in the flake8 checks for the packages I've modified.
- black and flake8 differ on documentation line lengths: black will move a triple-quote that closes an otherwise-single-line docstring to that first line if it's the only thing on its line; this already spurred
RFC-762, but it also causes complaints from flake8 (W505) when those quotes bring the length of the line over 79 chars. I haven't figured out a way to configure the tools to agree on this point without turning off flake8 checking of docstring line-lengths entirely, so I've just adjusted any docstrings that triggered this problem slightly to avoid it. Maybe using pydocstyle instead of flake8 to check doc line lengths would be an alternative?
Oh, and one more, brought to my attention by Tim Jenness: black will put whitespace-concatenated string literals on the same line if they fit, so constructs like:
raise SomeRatherLongExceptionType("Some text for that exception" |
" some more text.") |
often turn into
raise SomeRatherLongExceptionType(
|
"Some text for that exception" " some more text." |
)
|
I've been fixing this manually as well; I don't think it'll be a problem once we're in steady-state, and nothing breaks if we don't fix it, but it's a nice thing to do on any initial black format run.
I've added pex_config to the list – everyone treats it as middleware already.
If dependencies don't have mypy support you indicate that in the mypy.ini.
black+isort don't have any impact on dependencies so the decision to do that can happen any time. Waiting until we've sorted out butler and pipe_base is probably a good idea.
For mypy support in bps itself you need to build up slowly and that can probably wait until pipe_base and daf_butler are properly declared to support mypy.