This looks good to me. One thing we might want to consider is just having make format and make lint run both regular code and notebook versions of the shell scripts rather than adding a new make command specific to notebooks.

@eb8680 , any thoughts?

To pass lint, all imports are moved to the top of the file, and two function names are modified. Other changes are the result of auto-formatting.

I haven't used nbqa before. Just so I understand, what failure related to the imports wasn't fixed by autoformatting with isort and black? Does this mean running nbqa isort --check <filename> can still fail even if you run nbqa isort <filename> first?

One thing we might want to consider is just having make format and make lint run both regular code and notebook versions of the shell scripts rather than adding a new make command specific to notebooks.

I think it's OK to keep the notebook part separate for now.

To pass lint, all imports are moved to the top of the file, and two function names are modified. Other changes are the result of auto-formatting.

I haven't used nbqa before. Just so I understand, what failure related to the imports wasn't fixed by autoformatting with isort and black? Does this mean running nbqa isort --check <filename> can still fail even if you run nbqa isort <filename> first?

I think this is more about flake8 being included in the lint, it sometimes complains about things that make clean doesn't fix, like lines too long or unused imports. I've seen this happen outside of nbqa as well sometimes. In the case of notebooks it also seems that isort sorts imports within cells, but if some imports are not in the top cell, flake8 complains.

Relatedly, I resolved this in the collab repo by adding autoflake --remove-all-unused-imports --in-place ... to the cleaning script. I assume we do not want to remove unused imports in modules as, for example, this is a bit annoying for modules that are WIP, but perhaps could be useful for notebook cleaning, as notebooks will be only added to the list once assumed done?

Relatedly, I resolved this in the collab repo by adding autoflake --remove-all-unused-imports --in-place ... to the cleaning script. I assume we do not want to remove unused imports in modules as, for example, this is a bit annoying for modules that are WIP, but perhaps could be useful for notebook cleaning, as notebooks will be only added to the list once assumed done?

Yes, I think it would be a mistake to have make clean automatically remove unused imports. My preference is that we automate things that developers will almost never care about, like import orders or trailing whitespace, but avoid being too heavy handed beyond that.

Anyway, who would you like to review this @rfl-urbaniak ? I always find it confusing when there are two reviewers listed.

Sorry for being confusing. I wanted @eb8680 to approve the general strategy, as this is what would be applied across all notebooks in the repo, and @SamWitty to approve the resulting changes to the notebook,

Sorry for being confusing. I wanted @eb8680 to approve the general strategy, as this is what would be applied across all notebooks in the repo, and @SamWitty to approve the resulting changes to the notebook,

Ok, thanks for clarifying. The changes to the notebook have my stamp of approval. @eb8680 , ball's in your court to review the broader PR.