+1 for autopep8 On Thu, Apr 9, 2020 at 4:45 PM Wes McKinney <wesmck...@gmail.com> wrote:
> So to summarize, it seems that what we are agreeing in this thread is > to not debate readability about otherwise PEP8-compliant Python code > in code reviews, is that right? Absent a consensus about a change, the > outcome is basically "no change". The addition of autopep8 as a tool > for automated PEP8 fixes (as opposed to manual ones, which is what's > required right now) seems reasonable in that it does not make invasive > changes to the current codebase. > > We can also abandon this discussion and not use any tool. Either way > it would be good to reach a conclusion and move on. > > - Wes > > On Thu, Apr 9, 2020 at 3:05 AM Uwe L. Korn <uw...@xhochy.com> wrote: > > > > The non-configurability of black is one of the strongest arguments I see > for black. The codestyle will always be subjective. From previous > discussions I know that my personal preference of readability conflicts > with that of Antoine and Wes, so will probably others. We have the same > issue with using the Google C++ style guide in C++. Not everyone agrees > with it but at least it is a guide that is adopted widely (and thus seen in > other projects quite often) and has well outlined reasonings for most of > its choices. > > > > On Wed, Apr 8, 2020, at 10:25 PM, Xinbin Huang wrote: > > > Another option that we can look into is yapf ( > > > https://github.com/google/yapf). It is similar to black but more > tweakable. > > > Also, it is recently adopted by the Apache Beam project. PR is here > > > https://github.com/apache/beam/pull/10684/files > > > > > > Bin > > > > > > On Wed, Apr 8, 2020 at 1:18 PM Wes McKinney <wesmck...@gmail.com> > wrote: > > > > > > > I don't think it's possible unfortunately. From the README: "Black > > > > reformats entire files in place. It is not configurable." > > > > > > > > The main concern about Black is the impact that it has on > readability. > > > > I share this concern as the subjective style choices it makes are > > > > quite different from the way I've been writing Python code the last > 12 > > > > years. That doesn't mean I'm right and it's wrong, of course. In this > > > > project, it doesn't seem like we're losing much energy to people > > > > reformatting arguments lists or adding line breaks here and there, > > > > etc. FWIW, from reading the Black docs, it doesn't strike me that the > > > > tool is designed to maximize readability, but rather to make > > > > formatting deterministic and reduce code diffs caused by > reformatting. > > > > > > > > My opinion is that we should simply avoid debating code style in code > > > > reviews as long as the code passes the PEP8 checks. Employing > autopep8 > > > > is an improvement over the status quo (which is that developers must > > > > fix flake8 / PEP8 warnings by hand). We can always revisit the Black > > > > discussion in the future if we find that subjective code formatting > > > > (beyond PEP8 compliance) is taking up our energy inappropriately. > > > > > > > > On Wed, Apr 8, 2020 at 2:13 PM Rok Mihevc <rok.mih...@gmail.com> > wrote: > > > > > > > > > > Could we 'tone down' black to get the desired behavior? > > > > > I'm ok with either tool. > > > > > > > > > > Rok > > > > > > > > > > On Wed, Apr 8, 2020 at 8:00 PM Wes McKinney <wesmck...@gmail.com> > wrote: > > > > > > > > > > > On Wed, Apr 8, 2020 at 12:47 PM Neal Richardson > > > > > > <neal.p.richard...@gmail.com> wrote: > > > > > > > > > > > > > > So autopep8 doesn't fix everything? Sounds inferior to me. That > > > > said, I'm > > > > > > > in favor of any resolution that increases our automation of > this and > > > > > > > decreases the energy we expend debating it. > > > > > > > > > > > > It does fix everything, where "everything" is compliance with > PEP8, > > > > > > which I think is the thing we are most interested in. > > > > > > > > > > > > Black makes a bunch of other arbitrary (albeit consistent) > > > > > > reformattings that don't affect PEP8 compliance. > > > > > > > > > > > > > Neal > > > > > > > > > > > > > > > > > > > > > On Wed, Apr 8, 2020 at 10:34 AM Wes McKinney < > wesmck...@gmail.com> > > > > > > wrote: > > > > > > > > > > > > > > > Circling back on this, it seems there isn't consensus about > > > > switching > > > > > > > > to Black, and using autopep8 at least will give us an easy > way to > > > > > > > > maintain PEP8 compliance and help contributors fix linting > failures > > > > > > > > detected by flake8 (but not all, e.g. unused imports would > need to > > > > be > > > > > > > > manually removed). Would everyone be on board with using > autopep8? > > > > > > > > > > > > > > > > On Thu, Apr 2, 2020 at 9:07 AM Wes McKinney < > wesmck...@gmail.com> > > > > > > wrote: > > > > > > > > > > > > > > > > > > I'm personally fine with the Black changes. After the > one-time > > > > cost > > > > > > of > > > > > > > > > reformatting the codebase, it will take any personal > preferences > > > > out > > > > > > > > > of code formatting (I admit that I have several myself, > but I > > > > don't > > > > > > > > > mind the normalization provided by Black). I hope that > Cython > > > > support > > > > > > > > > comes soon since a great deal of our code is Cython > > > > > > > > > > > > > > > > > > On Thu, Apr 2, 2020 at 9:00 AM Jacek Pliszka < > > > > > > jacek.plis...@gmail.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > Hi! > > > > > > > > > > > > > > > > > > > > I believe amount of changes is not that important. > > > > > > > > > > > > > > > > > > > > In my opinion, what matters is which format will allow > > > > reviewers > > > > > > to be > > > > > > > > > > more efficient. > > > > > > > > > > > > > > > > > > > > The committer can always reformat as they like. It is > harder > > > > for > > > > > > the > > > > > > > > reviewer. > > > > > > > > > > > > > > > > > > > > BR, > > > > > > > > > > > > > > > > > > > > Jacek > > > > > > > > > > > > > > > > > > > > czw., 2 kwi 2020 o 15:32 Antoine Pitrou < > anto...@python.org> > > > > > > > > napisał(a): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > PS: in both cases, Cython files are not processed. > autopep8 > > > > is > > > > > > > > actually > > > > > > > > > > > able to process them, but the comparison wouldn't be > > > > > > > > apples-to-apples. > > > > > > > > > > > > > > > > > > > > > > (that said, autopep8 gives suboptimal results on Cython > > > > files, > > > > > > for > > > > > > > > > > > example it changes "&c_variable" to "& c_variable" and > > > > > > > > > > > "void* ptr" to "void * ptr") > > > > > > > > > > > > > > > > > > > > > > Regards > > > > > > > > > > > > > > > > > > > > > > Antoine. > > > > > > > > > > > > > > > > > > > > > > Le 02/04/2020 à 15:30, Antoine Pitrou a écrit : > > > > > > > > > > > > > > > > > > > > > > > > Hello, > > > > > > > > > > > > > > > > > > > > > > > > I've put up two PRs to compare the effect of running > black > > > > vs. > > > > > > > > autopep8 > > > > > > > > > > > > on the Python codebase. > > > > > > > > > > > > > > > > > > > > > > > > * black: https://github.com/apache/arrow/pull/6810 > > > > > > > > > > > > 65 files changed, 7855 insertions(+), 5215 > deletions(-) > > > > > > > > > > > > > > > > > > > > > > > > * autopep8: > https://github.com/apache/arrow/pull/6811 > > > > > > > > > > > > 20 files changed, 137 insertions(+), 118 > deletions(-) > > > > > > > > > > > > > > > > > > > > > > > > I've configured black to try and minimize changes > (for > > > > example, > > > > > > > > avoid > > > > > > > > > > > > normalizing string quoting style). Still, the > number of > > > > > > changes is > > > > > > > > > > > > humongous and they add 2600 lines to the codebase > (which > > > > is a > > > > > > > > tangible > > > > > > > > > > > > amount of vertical space). > > > > > > > > > > > > > > > > > > > > > > > > Regards > > > > > > > > > > > > > > > > > > > > > > > > Antoine. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
