add isort workflow

[mirpedrol]

Attempt to implement isort in CI as mentioned in #1526 comment.
Adds isort to sort and organize python imports as a GH action workflow.

PR checklist

• This comment contains a description of changes (with reason)
• CHANGELOG.md is updated
• If you've fixed a bug or added code that should be tested, add tests!
• Documentation in docs is updated

[ewels]

Great stuff, thanks for this! What do you think as adding it as a job to the existing lint-code.yml file? As it's conceptually similar (checking code quality).

Also I would expect there to be a bunch of code changes required after adding this? 😅

[mirpedrol]

That's true, I will add it to lint-code.yml instead of creating a new file, thanks!
Should I add the changes in the code after running isort in this same PR?

[ewels]

Should I add the changes in the code after running isort in this same PR?

Yes please, as otherwise tests on the repo will immediately start failing 😁 May as well do it all in one..

Note that the CI test should be failing on this PR but isn't: https://github.com/nf-core/tools/runs/6274840729?check_suite_focus=true

Looks like it's changing loads of stuff but then still reporting a ✅

[ewels]

Also it looks like jamescurtin/isort-action has been renamed to isort/isort-action

[fabianegli]

It would be great to have the isort configuration added to pyproject.toml. I would like to see three things specifically, one is the force single line imports another is to have nf_core set as known first party and the last is profile black for compatibility with black.

[tool.isort]
profile = "black"
known_first_party = ["nf_core"]
force_single_line = true

[mirpedrol]

I don't know why the linting is failing after running isort. It's successful for me locally with isort --check-only --diff . or isort --profile black --multi-line 3 --project nf-core --check-only --diff . but it fails in the GitHub action. Any idea? @ewels @fabianegli

I also added isort to fix-linting.yml, it works locally with act, but I don't fully trust it. I guess it can't be tested before based on this

This should be ready for review once I solve the linting error here :)

[ewels]

I'm getting more tempted to use precommit... 👀

[ewels]

Looking good! Please can you add some docs about how contributors can run this locally before pushing etc?

[fabianegli]

+1 For documenting how to run isort locally. (I think at some point we might want to provide .pre-commit-hooks for convenience, but I see that as a separate issue from adding isort linting.)

I think the docs can just mention running isort . in the top level directory of the tools git repo and link to https://pycqa.github.io/isort/index.html for further reference.

[mirpedrol]

Please can you add some docs about how contributors can run this locally before pushing etc?

I am not sure on the best place to add the documentation. Would you be OK with adding a new section "Contributing" and "Code formatting" to the README?

[fabianegli]

There's also a possibility to create a specific place for developer documentation, e.g. a CONTRIBUTING file like the one in biopython. I would say the README is fine though. But I wonder what @ewels thinks?

[ewels]

You mean like this? 😉 https://github.com/nf-core/tools/blob/master/.github/CONTRIBUTING.md

It shows up as a hint when you open issues and PRs but yeah, we should probably move out into the root and mention it in the readme to make it more findable.

[fabianegli]

I made a new issue for your proposals and think for this PR the docs can simply be added to .github/CONTRIBUTING.md

[fabianegli]

for black, there's no effect description in the dev-docs, and I think its not necessary. I also think it is not necessary for the isort. It just does its thing. If it needs to be desribed, I would go with something like this: "isort separates imports into sections by origin (builtin > third party > first party) and within the sections by type (import > import from) and within type alphabetically." but that still doesn't capture the indentation for multiple imports... so I would just skip that.

[ewels]

Great stuff @mirpedrol 👏🏻 Thank you!

[fabianegli]

Thank you @mirpedrol
It's so nice to have isort integrated handling the imports 😍