Skip to content
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

feat: running black and isort over codebase #4

Closed
wants to merge 0 commits into from

Conversation

oriordan
Copy link
Contributor

This commit also introduce the github workflows to run pre-commit hooks on push and pull requests.

@oriordan
Copy link
Contributor Author

So before I go and resolve conflicts - do you accept this direction or not? I had a few changes piling up waiting for this. I think it makes code collab much easier if we grab a set of linters early and stick with those. black, isort, pylint, ruff are pretty much standard these days.

Once we get through that, we can get rid of the prints and shape some more or less stable interface for the core.

Copy link
Owner

@PriNova PriNova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @oriordan, thanks for the contribution. I've reviewed and accepted the changes.

Moving forward, I'm considering updating the linting hook to:

  1. Implement a matrix strategy for pinning specific Python versions.
  2. Cache the pre-commit environment to enhance performance.

What do you think?

pyproject.toml Outdated
"black",
"isort",
"pylint",
"ruff",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't ruff superfluous because of already using pylint and the others? ruff has also issued to handle f-strings in Python 3.12 incorrectly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gitlab is trolling me, I have not seen your comment 5 days ago and I explicitly checked. Sorry for that.
Somewhat yes, I personally believe ruff is the future and want to keep running it along with pylint which is much more robust right now (and slow as well). I have only seen a very few marginal things caught by ruff which was not by pylint, and that was likely due to the pylint config we use. If you don't mind, keep it there, otherwise we can stick to pylint.

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.

2 participants