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

Use code styler #672

Open
2 of 6 tasks
niccokunzmann opened this issue Jun 30, 2024 · 5 comments
Open
2 of 6 tasks

Use code styler #672

niccokunzmann opened this issue Jun 30, 2024 · 5 comments

Comments

@niccokunzmann
Copy link
Member

niccokunzmann commented Jun 30, 2024

In order for the codebase to have a common style, we can use a linter/formatter like black or ruff.

While black allows us to format the codebase, ruff can enforce more compliance like import order, no prints and more.

To close this issue:

  • choose ruff or black
  • add a badge
  • document how to install the formatter/linter in a pre-commit hook. example
  • create CI tests to either fail if not formatted or commit formatted code. example
  • format all code files
  • last: remove optional test result, fail on mistakes Ruff formatting #680 (comment)

See also:

@stevepiercy
Copy link
Member

This is a good unopinionated FAQ to help choose between tools: https://docs.astral.sh/ruff/faq/#how-can-i-disableforce-ruffs-color-output

I'd go with ruff, since we don't have a linter or formatter at this point, at least none that I know of.

I'd also suggest this issue be done before 6.0.0 final, because it will likely generate a huge diff.

document how to install the formatter/linter in a pre-commit hook. example

I don't see a .pre-commit-config.yaml file, so does this statement imply that there needs to be another step for developers in the documentation to install pre-commit?

@niccokunzmann
Copy link
Member Author

I don't see a .pre-commit-config.yaml file, so does this statement imply that there needs to be another step for developers in the documentation to install pre-commit?

correct.

I am ok with ruff. If I create a PR, I am very happy to have contributions to it as it will be a lot of effort. I did it recently for the Open Web Calendar.

@stevepiercy
Copy link
Member

I would also like contributions to provide a reason for using it as a comment in the configuration.

Line length on narrative text in documentation (not docstrings) does not make any sense, and it makes it difficult to do reviews due to rewrapping text to an arbitrary line length. Who cares? When rendered to HTML or other formats, line wraps go away. One sentence per line is a good guide, and what we use in Plone.

Please do begin with what you have from Open Web Calendar, and let's iterate over it. Thank you!

@niccokunzmann
Copy link
Member Author

#667 should be merged before and I would ask @stevepiercy, @geier, @angatha and @jacadzaca:

Would you have time to work on this together? When? This big diff will basically conflict with all PRs that are going on.

@stevepiercy
Copy link
Member

I'd suggest start work on the configuration now, so we can see what both local testing and CI reveal without actually changing files. We can ignore local test and CI failures as "experimental" for a while. When there is an alpha release of 6.0.0, I assume that the PRs would slow down, then we would apply the configuration and write files in successive PRs, applying each format and lint configuration item separately.

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

No branches or pull requests

2 participants