Skip to content

fix charm4py formatting and add check #305

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

fix charm4py formatting and add check #305

wants to merge 9 commits into from

Conversation

ritvikrao
Copy link
Collaborator

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.

@matthiasdiener
Copy link
Contributor

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.

@ritvikrao
Copy link
Collaborator Author

ritvikrao commented Jul 2, 2025

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.

adityapb
adityapb previously approved these changes Jul 2, 2025
@adityapb adityapb dismissed their stale review July 2, 2025 14:34

We should probably discuss in the meeting whether it will cause any issues tracking changes with git blame. But otherwise seems like a good idea to use a linter

@matthiasdiener
Copy link
Contributor

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.

@ritvikrao
Copy link
Collaborator Author

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.

@matthiasdiener
Copy link
Contributor

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?

@ritvikrao
Copy link
Collaborator Author

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

@matthiasdiener
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants