EmilStenstrom / django-components

Create simple reusable template components in Django.
MIT License
973 stars 62 forks source link

Add automatic code formatting with Black #36

Closed EmilStenstrom closed 2 years ago

EmilStenstrom commented 3 years ago

I would like to not have to consider coding style at all, and this is the problem that black solves. This feature would mean that we add black as a dev dependency, and add a github action that checks that the code is formatted before passing.

danjac commented 3 years ago

Maybe add as pre-commit along with isort ?

EmilStenstrom commented 3 years ago

Yes! Flake8 too.

hanifbirgani commented 2 years ago

So we should use black, isort, etc. as pre-commit on the developer-side and a flake8 github action at the repo-side 🤔

EmilStenstrom commented 2 years ago

I think so. I don't think the first version is that important, we can iterate later.

Is it possible to run black, isort etc in "check mode" on the repo side? I'd like to ensure that all commits follow a set standard, and I think that pre-commit requires that developers have installed a specific hook before commiting? Seems easy to miss unless this is enforced before commit is allowed.

hanifbirgani commented 2 years ago

Pre-commit hooks could run before any commit on developer-side: https://pre-commit.com/#3-install-the-git-hook-scripts

So a flake8 GitHub action would be enough in my opinion.

EmilStenstrom commented 2 years ago

The scenario I'm thinking about is that some new developer comes along that wants to help with developing django-component. This developer does not know the setup that's needed. They may not install the pre-commit hooks locally at all, or might even edit files using the GitHub interface. Am I wrong that no developer hooks would be run in that case?

EmilStenstrom commented 2 years ago

Perhaps we should use https://pre-commit.ci/ which seems to also fix things when developers miss to run tools directly in the PR?

hanifbirgani commented 2 years ago

Based on my research, Github doesn't allow changing forked pull requests' code, so the action we need here should be applied to the main branch after merging PRs. I work on the action and I will post the progress here.

EmilStenstrom commented 2 years ago

Isn't changing the code on a forked PR just what pre-commit.ci does? It seems to show up on all PR:s posted to a repo, run the tools you tell it to, and if they generate diffs apply those diffs in a new commit.

hanifbirgani commented 2 years ago

Isn't changing the code on a forked PR just what pre-commit.ci does?

My previous comment is about Github action limitations. I think pre-commit.ci works as a bot with access to edit forked PRs. 🤔

I will add pre-commit configurations to the repo as new PR.

hanifbirgani commented 2 years ago

@EmilStenstrom Would you mind merging #90 and signing in to pre-commit.ci with your Github account, then give it access to this repo to run pre-commit (isort, black, and flake8) config?

EmilStenstrom commented 2 years ago

@hanifbirgani Thanks for the reminder. I've now done both of the things you requested!

hanifbirgani commented 2 years ago

I guess pre-commit.ci needs a new merge request on the master branch to make changes 🤔

EmilStenstrom commented 2 years ago

@hanifbirgani What does that mean, that someone puts up a new PR?

hanifbirgani commented 2 years ago

@EmilStenstrom I thought we could make a new PR to put pre-commit.ci service on a test run.

EmilStenstrom commented 2 years ago

Yes, go ahead! Feel free to experiment all you want to get this working.

EmilStenstrom commented 2 years ago

This is now added thanks to @hanifbirgani! Black, isort, flake8 are all enforced via a pre-commit hook, and also via pre-commit.ci which will autofix PR:s which did not install the pre-hooks.