charm4py icon indicating copy to clipboard operation
charm4py copied to clipboard

fix charm4py formatting and add check

Open ritvikrao opened this issue 7 months ago • 7 comments

This fixes all formatting in the codebase to PEP 8 standards, and uses the super-linter repo to enforce the formatting in Github Actions. The Black tool can be used to fix any formatting issues.

ritvikrao avatar Jun 30 '25 17:06 ritvikrao

I think adding a linter to CI is great. A couple of suggestions:

  • I think ruff itself has become a super-linter; in particular, it contains the relevant rules for the other Python linters from https://github.com/super-linter/super-linter, so I suggest running just ruff. pip install ruff && ruff check should be enough.
  • Please move the CI config to the main CI file, charm4py.yml. This makes it easier to sync configurations across different CI tests.

matthiasdiener avatar Jul 01 '25 21:07 matthiasdiener

I moved the CI test into the main CI file. However, i tried using ruff on my local machine and I think it's too aggressive; it checks for things like undefined variables/star imports/other problems that I am worried will cause the code to break. I did apply some of the suggested ruff fixes here but I think the CI should just check for formatting.

For further context ruff check resulted in 422 errors in the codebase.

ritvikrao avatar Jul 02 '25 13:07 ritvikrao

I moved the CI test into the main CI file. However, i tried using ruff on my local machine and I think it's too aggressive; it checks for things like undefined variables/star imports/other problems that I am worried will cause the code to break. I did apply some of the suggested ruff fixes here but I think the CI should just check for formatting.

For further context ruff check resulted in 422 errors in the codebase.

Hmm, that is confusing, since https://github.com/super-linter/super-linter appears to include ruff as well? Can you disable the errors that you think are too aggressive globally in pyproject.toml (see e.g. https://github.com/inducer/meshmode/blob/89fcb4955b160098e23389801454b9e90fb91e25/pyproject.toml#L74 for an example). My concern about using a github-actions specific formatting check is that it makes it harder to reproduce error checks locally.

matthiasdiener avatar Jul 02 '25 14:07 matthiasdiener

I already have done that, it only checks for Black-based formatting. The way the super linter works is that there a bunch of "VALIDATE_" environment variables that check for things, and if you set one to TRUE (like setting VALIDATE_PYTHON_BLACK to true), it will turn off all the other checks. You can also see in the actions that only the Black check is being run.

ritvikrao avatar Jul 02 '25 14:07 ritvikrao

I already have done that, it only checks for Black-based formatting. The way the super linter works is that there a bunch of "VALIDATE_" environment variables that check for things, and if you set one to TRUE (like setting VALIDATE_PYTHON_BLACK to true), it will turn off all the other checks. You can also see in the actions that only the Black check is being run.

Could you just run ruff format then instead of ruff check?

matthiasdiener avatar Jul 02 '25 14:07 matthiasdiener

Ruff format is a bit different than black, and checks for both will have different results. Which one is best?

ritvikrao avatar Jul 02 '25 14:07 ritvikrao

Ruff format is a bit different than black, and checks for both will have different results. Which one is best?

Hmm, they should be almost the same (https://docs.astral.sh/ruff/faq/). I would go with ruff format, so we can easily "graduate" to do additional checks in the future.

matthiasdiener avatar Jul 02 '25 14:07 matthiasdiener