esrg-knights / Squire

A re-introduction of the WebApp for ESRG Knights of the Kitchen Table.
https://www.kotkt.nl/
GNU General Public License v3.0
11 stars 8 forks source link

Add Black linter code suggestions #306

Closed LenaWil closed 1 year ago

LenaWil commented 1 year ago

Fixes #13.

We should probably discuss more what kind of linter we want and how we would implement it without giving everyone merge conflicts, but here is a concept.

I went with adding black as a GitHub action that only checks the files changed in the pull request itself, setting it up to check only lines was complicated, and also as a GitHub action that checks all of them. Please say if I should remove the last one. Furthermore I added a review bot for it; in the case the code is only a few lines wrongly formatted it would be easier to fix. However, the permissions don’t work correctly with the main repo, so I need to check that with someone with more permissions, if we even want it.

LenaWil commented 1 year ago

Okay, that didn’t work like it was supposed to.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (91de163) 95.69% compared to head (a674ad1) 95.69%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #306 +/- ## ======================================= Coverage 95.69% 95.69% ======================================= Files 139 139 Lines 5780 5780 Branches 918 918 ======================================= Hits 5531 5531 Misses 170 170 Partials 79 79 ``` | Flag | Coverage Δ | | |---|---|---| | unittests | `95.69% <100.00%> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=esrg-knights#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/esrg-knights/Squire/pull/306?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=esrg-knights) | Coverage Δ | | |---|---|---| | [utils/auth\_utils.py](https://codecov.io/gh/esrg-knights/Squire/pull/306?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=esrg-knights#diff-dXRpbHMvYXV0aF91dGlscy5weQ==) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=esrg-knights). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=esrg-knights)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

LenaWil commented 1 year ago

Naar editorconfig kijken

EricTRL commented 1 year ago
  1. I don't think we want the lint-check-full job to run, as it looks for linting problems across the whole repository. The resulting workflow output is so large that it doesn't offer any clarity. We can reconsider this in the future, when we will probably have less linting problems.

  2. I can't tell whether lint-changed is actually working. Perhaps you could add a (purposefully wrongly formatted) Python file to this PR to showcase what the workflow would do/show? Food for thought: Is it needed to have this run on every push, instead of just PRs?

  3. Is there any extension/bot that'll need to be installed for these workflows? If so, I can add them.

LenaWil commented 1 year ago
  1. I don't think we want the lint-check-full job to run, as it looks for linting problems across the whole repository. The resulting workflow output is so large that it doesn't offer any clarity. We can reconsider this in the future, when we will probably have less linting problems.

Do you want me to remove the code, or just put if: false there?

  1. I can't tell whether lint-changed is actually working. Perhaps you could add a (purposefully wrongly formatted) Python file to this PR to showcase what the workflow would do/show?

Can do that?

Food for thought: Is it needed to have this run on every push, instead of just PRs?

I could disable it? If you want to?

I had enabled it because it only checks for the changes in one commit when running on pushes, and thus is different from the pull requests run.

  1. Is there any extension/bot that'll need to be installed for these workflows? If so, I can add them.

lint-git-pr-comments-action-suggester needs reviewdog, but later I was thinking how useful it would really be, because it only reacts on the lines of the files and not the rest of them. Then I was thinking if formatting the files wouldn’t generate a lot of noise in the diffs of pull requests, but I didn’t really have a better solution, other than doing everything in one try.

LenaWil commented 1 year ago

Speaking of, there are multiple ways to to configure reviewdog, like by code suggestions, or just warnings on the files themselves. Any preferences?