Add linting workflow using Ruff for pull requests

[arash77]

No description provided.

[hmenager]

I like this proposition, but should we first lint all the python code before merging this PR?

[mihai-sysbio]

If possible, it would be great if the workflow would automatically propose fixes as a new PR.

[jmfernandez]

I like the idea, extended not only to pull requests but all the commits.

Just as a proof of concept, I have cloned the repo and tested to run ruff from command line just with the same parameters which appear in the proposed CI workflow. ruff reports 171 errors, and 129 could be automatically fixed. So, it is very useful to point out what it is flaky in the code.

[jmfernandez]

Should checks also happen on push?

[hmenager]

I think they should. Out of curiosity, to which extent do you think this should be enforced:

• if errors are reported, just report them but do not forbid merging
• if severe errors are reported, forbid merging
• if any error is reported, forbid merging
  Bear in mind I'm not familiar enough with ruff, hence my question: I'd like us to adopt the "pragmatic best practices" level, but I'm not sure which one is.

[arash77]

What we can do is auto-fix on push to main (including merged PRs) by creating a separate PR that lints and formats the code. However, that will create a lot of messy PRs. It would be better if we could force the PR authors to lint and format the code themselves.

[jmfernandez]

Answering @hmenager comment #40 (comment)

I think they should. Out of curiosity, to which extent do you think this should be enforced:

* if errors are reported, just report them but do not forbid merging

This is a must at the beginning.

* if severe errors are reported, forbid merging

This behaviour should be agreed at least for pull requests.

* if any error is reported, forbid merging

Once we are accepting the former (forbidding the merge of pull requests failing in the validation) we have to enforce this one to avoid false positives. But, the only effective way to do it is protecting the main branch, forbidding direct commits.

  Bear in mind I'm not familiar enough with ruff, hence my question: I'd like us to adopt the "pragmatic best practices" level, but I'm not sure which one is.

ruff is a very useful tool among many others. It covers several scenarios, and it is very, very fast. But in some projects I have found that it is not as exhaustive as pylint, for instance.

So, I recommend an adoption roadmap of better practices, taking into account that "best" practices are a moving target resembling a ghost.

[jmfernandez]

Answering @arash77 comment #40 (comment)

What we can do is auto-fix on push to main (including merged PRs) by creating a separate PR that lints and formats the code. However, that will create a lot of messy PRs. It would be better if we could force the PR authors to lint and format the code themselves.

When I was thinking on automatic fixes, it was more about fixes to issues in the code itself (i.e. the fixes applied by ruff check --fix). But code formatting fixes make more sense, as they are not going to involve a change in the code behaviour.

[arash77]

@jmfernandez So should we do only auto format on push into the main by creating a separate PR?

[arash77]

There is also another tool that could suggest the changes right in the PR if it is related to the changes in the PR. https://github.com/reviewdog/reviewdog

[jmfernandez]

There is also another tool that could suggest the changes right in the PR if it is related to the changes in the PR. https://github.com/reviewdog/reviewdog

It might be "a bit too much", but it is worth a try

[jmfernandez]

@jmfernandez So should we do only auto format on push into the main by creating a separate PR?

I would start with that, in case there is no error in the analysed code (it does not make sense to propose a PR for code with errors)

[arash77]

There is also another tool that could suggest the changes right in the PR if it is related to the changes in the PR. https://github.com/reviewdog/reviewdog

It might be "a bit too much", but it is worth a try

It will look like this: arash77#4

[arash77]

The auto fix PR could look like this: arash77#5

[jmfernandez]

The auto fix PR could look like this: arash77#5

Looks quite promising @arash77 !!

[arash77]

But this PR doesn't include the auto fix, it is only to create the checks on PRs. Do we want it here?

[jmfernandez]

But this PR doesn't include the auto fix, it is only to create the checks on PRs. Do we want it here?

From my point of view, we should go step by step. First, the current purpose of this PR, which is to integrate the initial checks on every PR, should be initially accepted and then refined. In parallel, in order to avoid misleading fail checks, either one of utils author on behalf all of them, or each one of the utils authors, should manually apply the needed changes to improve the code quality of each hosted utility used for the Research Software Ecosystem.

What do you think, @hmenager (and other readers of this dialogue)?

[bgruening]

I would go the other way around:

• include a Makefile that does the lints and auto-format, auto-fix for developers.
• include a pre-processing hook for developers to use
• ruff the entire code base in one-commit (easier to skip during git blame etc)
• enable CI

I would not make any PR comments about that. This is just noise, no one wants to go line by line through this, people should use the Makefile or the pre-commit hook. Overall we also need to keep the infrastructure simple.

[arash77]

We could also use this pre-commit hook: https://github.com/astral-sh/ruff-pre-commit

[arash77]

I have added the Makefile so you can use make check and make fix.

[hmenager]

Code is fixed in #50 following advice from @bgruening , any review is appreciated 🙂

[arash77]

I will rebase this to see if the check is working correctly.

[hmenager]

Thanks @arash77 ! Let's merge it!