From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id gK3EFGX9hmAFawAAWB0awg (envelope-from ) for ; Mon, 26 Apr 2021 13:50:29 -0400 Received: by simark.ca (Postfix, from userid 112) id 4DC011F11C; Mon, 26 Apr 2021 13:50:29 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 31F071E54D for ; Mon, 26 Apr 2021 13:50:28 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id E9D203892451; Mon, 26 Apr 2021 17:50:27 +0000 (GMT) Received: from mail-wr1-x42b.google.com (mail-wr1-x42b.google.com [IPv6:2a00:1450:4864:20::42b]) by sourceware.org (Postfix) with ESMTPS id B64BD3838000 for ; Mon, 26 Apr 2021 17:50:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org B64BD3838000 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wr1-x42b.google.com with SMTP id h4so47751945wrt.12 for ; Mon, 26 Apr 2021 10:50:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=ow6BKuQIH+QbrO0DhM80nbURfJirJTU3nGpoQWGZCT0=; b=Qj4s5yVbr8v70n12Q/ibvK8nOi1N8M5BKJMjcwLto5ljdiAj//zk0gHeMdWfVqpfaL /OFkqT+R4py5A+Z8aBvz2fa67Qwu8A8cl2cHZ6cReb16Y8YcknPwbewWEkuGMuFLYvgg BZC8g2ND/hhRtCdOfMzSVPWOHyKikK94SnRkZdj1WtGWu/amKVtF4RWp5YG2iE99fA1s 6FPFTYveJglAu1nud0EAMF47IoSLnQ6o5JvfezaQOLUn55YmXtuv3exNWi6nTEwlRzTf jRVmthnl0AcxUCTnJX22pKcS7GKmIQAZqUflHhyDg3Qs6Eqwwd+nXo2dCwq3bRtzztO3 LM4w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=ow6BKuQIH+QbrO0DhM80nbURfJirJTU3nGpoQWGZCT0=; b=DQntTNZmIHtnOATBDZPtr9vpYmJWoUGrUNABfMsP0/9sAywIm8Q47xMX7+8j+5jzNE J53PORkucb+adbV0hJu5OnCaXTfPSnQ2j32SpdMapHTDmVmxpNEiataaXlORLE01xCp4 lm7a0PjVspSOQYjhEOHq5nZeDoJFp1vPIq5Wv1r9jt9brWLW1JB1gS/CU0xZlLKXqtWf 70zRLrjlzKtPx+sDmFmgFaxAGiLAcEqKhO5Nh/I9EnmMJ4zYo9jREZcTuY+V1Yo0F9zC JcYD0/U2AqxGfTPD99bHBSvIzuvLrbfnl+B5e6SUEb8Zlh18xZlllrcQ+r2EM12n2HJ8 4KNw== X-Gm-Message-State: AOAM533eDAGt0wadi6AT5h9GnVzz+WA0wY+xe/ES/WVJ3u5dsgcuWsCz ZRJsEfbh56tPL0QI4I9C2vfiFA== X-Google-Smtp-Source: ABdhPJwo1ZqLhnTx9rErvcQr/z3CUP3DQzZHvYHA8G5b083ugaN170jAFFa6/V8gsAZWLYkSkfKrTQ== X-Received: by 2002:a5d:6041:: with SMTP id j1mr4266170wrt.374.1619459423789; Mon, 26 Apr 2021 10:50:23 -0700 (PDT) Received: from localhost (host109-151-46-70.range109-151.btcentralplus.com. [109.151.46.70]) by smtp.gmail.com with ESMTPSA id s8sm918398wrn.97.2021.04.26.10.50.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 26 Apr 2021 10:50:23 -0700 (PDT) Date: Mon, 26 Apr 2021 18:50:22 +0100 From: Andrew Burgess To: Simon Marchi Subject: Re: Proposal: format GDB Python files with black Message-ID: <20210426175022.GV2610@embecosm.com> References: <20210426162149.GU2610@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 18:35:25 up 16 days, 4:22, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: gdb-patches@sourceware.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" * Simon Marchi [2021-04-26 13:25:37 -0400]: > On 2021-04-26 12:21 p.m., Andrew Burgess wrote: > > * Simon Marchi via Gdb-patches [2021-04-26 11:55:21 -0400]: > > > >> Hi all, > >> > >> A few people I talked to about this and I have good experience with the > >> tool black to auto-format Python code. It is simple to use, fast and > >> reliable (if it detects that it changed the meaning of the program by > >> mistake, internal-errors out). I don't think I need to spell out the > >> advantage of using such a tool, but just in case: it removes all the > >> overhead of thinking about formatting when writing or reviewing code. > >> > >> My suggestion would be to format our code with black all at once. The > >> typical counter-argument to this is "it will be harder to use > >> git-blame". I don't think this is true, because you need to be able to > >> skip over commits anyway, and it's trivial to skip over a commit when > >> git-blaming using an interactive tool. But it's also possible to tell > >> git to ignore certain commits when git-blaming [2], so we can do that. > >> > >> I think the output of black is quite readable. When it does weird > >> stuff, it's generally because the lines / expressions are two long > >> anyway, and deserve to be split in multiple lines / expressions. Here's > >> a branch that shows how it would look like: > > > > In general, the idea of auto-formatting the code sounds great, but I > > would like to understand more about how the process would work. > > > > Right now, if I make a change I edit a file, git add, git commit, and > > finally, git push. At what point does the auto-formatting take place? > > Do I need to manually run the tool after editing the file? Is it > > done, well, automatically at some point? And if it is done > > automatically, then at what point in the process does this happen, and > > how does this relate to code review? > > Those are good questions. > > At the simplest level, the individual making changes is responsible for > the tool at any time they want, as long as it's before sending the patch > and / or pushing to master. So it would be pretty much like today, > where we are responsible for formatting the source code correctly, > except that it would now be tool-assisted instead of done mentally. > > How to run the tool is left to each individual: > > 1. By hand on the command line > 2. From the editor [1] > 3. Using a git-commit hook So without going more sophisticated as you describe below we'd still be relying on reviewers to either (with enough experience) "just know" that the code is black formatted, or apply the patch, run black, and see if the formatting changes. Then of course, there's the questions about what happens when different users are running different versions of black - assuming different versions might format things slightly differently. Another concern I'd have is that unrelated "formatting" changes might creep in. So, I edit a .py file and either don't run black (naughty), or use version X of black. Much later, I edit a different part of the same file, now either I run black, or run version Y of black. My original edit is reformatted slightly differently. My assumption is that this change (the second patch) should be rejected at review, as the reformatting change (of the earlier code) is unrelated to the work of the second patch. But I suspect some folk might find it frustrating, if on one had we say run black, but on the other hand we say you need to exclude unrelated chunks. I think, in the heterogeneous environments we all develop in, situations like the above will crop up, so it would be important to know what the expectations would be in such a case. > > If we want to be a bit more sophisticated, we could check-in a git > commit hook that runs black (if available) to check your formatting as > you are committing, if your commit includes changes to Python files. > That doesn't prevents formatting errors from slipping in the repo, but > it's already a good start (and formatting errors slipping in the repo > are not the end of the world). Again, this sort of thing works great in a corporate environment where you can guarantee that everyone is (or has no excuse not to be) running the exact same version of every tool. My concern would be that such a strategy would invite unintended changes to creep in. > > We don't really have CI right now (especially pre-merge testing), but if > we had, checking the formatting of Python files could be a validation > step. Agreed. > We do however, have scripts that run server side on push, so it would be > possible to run black server-side to reject commits that would introduce > formatting errors. I guess this would solve a lot of the problems I'm concerned about. Once we've "corrected" the formatting of all .py code then nobody could merge bad code. Thanks for all the info, Andrew