|
Hi all,
An alternative solution to the problem of nit-picky, formatting
related PR reviews is simple: let's not do that.
We already have an automated code formatting enforcer, flake8.
Let's simply drop the requirement on fixing anything that flake8
doesn't pick up. A committer can fix up style issues if they want
to, but shouldn't make anyone else do it. This would mean most of
the stuff on our coding
style page should just be delete, or at least not enforced -
by which I mean almost anything that can't be enforced by a tool
(such as isort, flake8, editors via .editorconfig etc.), and has
no non-local effects. (So consistent naming of classes/functions
*should* be enforced, because that affects other people's ability
to use the code). Large parts of that page are just duplicating
of flake8/isort rules anyway. Honestly, does it kill us if someone
writes "we" in a code comment? And black couldn't help with some
of these things anyway. Let's just stop being code review jerks.
I'm kind of ambivalent on black itself. Certainly there are cases
where it makes code less readable (a significant sin in my book)
e.g. lists that are better displayed vertically, as mentioned
already, and there are cases where it makes your diff larger than
it needs to be (e.g. when it decides a list is now too long and
needs to be re-formatted vertically). If we adopt black we'd have
to live with those annoyances. Alternatively, we can live with the
annoyance that code formatting is not perfectly consistent and we
accept less than 'perfect' PR. But we should just live with those
things:
A foolish consistency
is the hobgoblin of
little minds
And if our consistency requirements are causing problems for
people attempting to contribute, they are foolish and should be
dropped.
My 2 ¢.
Luke
On 18/04/2019 16:03, Jani Tiainen
wrote:
Well let me add my two cents here since I was also
in the group in DCEU that talked about the usage of black.
Personally I don't like to contribute to Django. And this
is why:
Day one: I'll make the fix/patch and create PR
Day two (or four or five depending how busy reviewers are):
I missed a comma or some minor indent is wrong
Day three: I fix styles
Day four: PR is accepted.
So whole round trip took about a five days (give a take few
usually depending how busy reviewers are).
That gives me a feeling that I'm really wasting my time and
since I can't get all the small bits and pieces exactly as
Django wants in correct place.
And that's because we have slightly different rules at the
work. And some other projects do have different rules.
So it would be great if some of this pain could be relieved
with a tool. In my short experience with black (I've been
using it for work projects) it does a pretty decent job.
Like others have said black does some decisions I don't
agree with. But I don't have to. Black does it for a "greater
good". And after a while black actually vanishes from the
flow.
Hi.
I propose that Django starts using 'black' [0] to auto-format
all Python code.
For those unfamiliar with 'black' I recommend reading the the
projects README.
The short version: it aims to reduce bike-shedding and non
value-adding
discussions; saving time reviewing code; and making the
barrier to entry lower
by taking some uncompromissing choices with regards to
formatting. This is
similar to tools such as 'gofmt' for Go and 'prettier' for
_javascript_.
Personally I first got involved contributing to Django couple
of weeks back,
and from anecdotal experience I can testify to how 'formatting
of code' creates
a huge barrier for entry. My PR at the time went multiple
times back and forth
tweaking formatting. Before this, I had to research the style
used by exploring
the docs at length and reading at least 10-20 different source
– and even those
were not always consistent. At the end of the day I felt like
almost 50% of the
time I used on the patch was not used on actually solving the
issue at hand.
Thinking about code formatting in 2019 is a mental energy
better used for other
things, and it feels unnecessary that core developers on
Django spend their time
"nit-picking" on these things.
I recently led the efforts to make this change where I work.
We have a 200K+
LOC Django code-base with more than 30K commits. Some key
take-aways: it has
drastically changed the way we work with code across teams,
new engineers are
easier on-boarded, PR are more focused on architectural
choices and "naming
things", existing PRs before migration had surprisingly few
conflicts and were
easy to fix, hot code paths are already "blameable" and it's
easy to blame a
line of code and go past the "black-commit", and lastly the
migration went
without any issues or down-time.
I had some really fruitful discussions at DjangoCon Europe
this week on this
very topic, and it seems we are not alone in these
experiences. I would love to
hear from all of you and hope that we can land on something
that will enable
*more* people to easier contribute back to this project.
I've set up how this _could_ look depending on some
configurables in Black:
* Default config: https://github.com/hermansc/django/pull/1
* Line length kept at 119: https://github.com/hermansc/django/pull/3
* Line length kept at 119, no string normalization:
https://github.com/hermansc/django/pull/2
Please have a look at the Black documentation. It explains the
benefits better
than I possibly could do here.
With kind regards,
Herman Schistad
[0]: https://github.com/ambv/black
--
You received this message because you are subscribed to the
Google Groups "Django developers (Contributions to Django
itself)" group.
To unsubscribe from this group and stop receiving emails from
it, send an email to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAN%3DnMTx0EE5WfXuccv_e3MBuCxp9u_pAV_ow5MxNST6MptTDBw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
--
Jani Tiainen
Software wizard
Always open for short term jobs or contracts to
work with Django.
--
You received this message because you are subscribed to the Google
Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it,
send an email to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAHn91ofkXhofPR_3bjSg2Swei2SyQ_axo6Ymy2dSuKg8G878GA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/d0ea7514-247a-90fa-e4cb-1c238b07ffc3%40cantab.net.
For more options, visit https://groups.google.com/d/optout.
|
