search

jazzband / django-invitations

Generic invitations app for Django
GNU General Public License v3.0
569 stars 166 forks source link

Add .pre-commit-config.yaml #175

Closed valberg closed 2 years ago

valberg commented 2 years ago

The git-commit is failing :(

Specifically: invitations/adapters.py:7:1: F401 'django.utils.encoding.force_str' imported but unused invitations/adapters.py:28:25: F821 undefined name 'force_text'

(i.e. force_text should be force_str)

This will be fixed when merging https://github.com/jazzband/django-invitations/pull/174 either before or after this PR.

Opinion: at this point, we keep the git-config - and this PR - small. There were changes in practically all pages because of Black, which makes reviewing it super hard. I will for sure let something slip by.

I'm thinking that's why we have tests?

Opinion: this is to be ignored if the PR is kept small, but I would remove migrations files from getting styled by Black as these are auto-generated

Good point! Adding it now!

MrCordeiro commented 2 years ago

I'm thinking that's why we have tests?

Tests are only good for detecting things we considered testing though.

I'm not very familiar with all those hooks. It makes me feel uncomfortable about them doing something unpredictable. Btw add-trailing-comma seems unnecessary with black in there.

valberg commented 2 years ago

I'm not very familiar with all those hooks. It makes me feel uncomfortable about them doing something unpredictable. Btw add-trailing-comma seems unnecessary with black in there.

I can vouch for the pre-commit configuration not doing any harm - I've been running with it on other project for quite some time now.

The benefits of add-trailing-comma can be read here: https://github.com/asottile/add-trailing-comma#multi-line-method-invocation-style----why

MrCordeiro commented 2 years ago

But Black already does all those benefits listed at those docs. Am I missing anything?

From: Víðir Valberg Guðmundsson @.> Sent: Wednesday, March 30, 2022 5:12 PM To: jazzband/django-invitations @.> Cc: Fernando @.>; Review requested @.> Subject: Re: [jazzband/django-invitations] Add .pre-commit-config.yaml (PR #175)

I'm not very familiar with all those hooks. It makes me feel uncomfortable about them doing something unpredictable. Btw add-trailing-comma seems unnecessary with black in there.

I can vouch for the pre-commit configuration not doing any harm - I've been running with it on other project for quite some time now.

The benefits of add-trailing-comma can be read here: https://github.com/asottile/add-trailing-comma#multi-line-method-invocation-style----why

— Reply to this email directly, view it on GitHubhttps://github.com/jazzband/django-invitations/pull/175#issuecomment-1083339707, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AE5E6K7PW53W5NT42X7GTDTVCR4O7ANCNFSM5SCHHZLQ. You are receiving this because your review was requested.Message ID: @.***>

valberg commented 2 years ago

But Black already does all those benefits listed at those docs. Am I missing anything?

Black reacts to trailing commas (by putting code with trailing commas on multiple lines) - add-trailing-comma adds them :)

valberg commented 2 years ago

@MrCordeiro @brylie Everything is green now :)

MrCordeiro commented 2 years ago

But look at the Black docs: it adds a comma too.

Screenshot_20220330-201852.jpg

Anyways this is a non blocker. If it's green, it's good.

valberg commented 2 years ago

But look at the Black docs: it adds a comma too.

Ahh that is just if the line is too long and has to wrapped. add-trailing-comma does it whatever the length of the line.

This ties into https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html?highlight=trailing%20comma#the-magic-trailing-comma

Anyway, great! I'll merge it!