From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id DF38E3838000 for ; Mon, 26 Apr 2021 18:08:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org DF38E3838000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 13QI8O7J009728 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 26 Apr 2021 14:08:29 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 13QI8O7J009728 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 861661E54D; Mon, 26 Apr 2021 14:08:24 -0400 (EDT) Subject: Re: Proposal: format GDB Python files with black To: Andrew Burgess Cc: gdb-patches@sourceware.org References: <20210426162149.GU2610@embecosm.com> <20210426175022.GV2610@embecosm.com> From: Simon Marchi Message-ID: Date: Mon, 26 Apr 2021 14:08:24 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: <20210426175022.GV2610@embecosm.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Mon, 26 Apr 2021 18:08:24 +0000 X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org 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: , X-List-Received-Date: Mon, 26 Apr 2021 18:08:32 -0000 On 2021-04-26 1:50 p.m., Andrew Burgess wrote: > 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. The use of black would be documented somewhere (in the repo, in the wiki, or both). Nevertheless, I don't expect all contributors to know about it and do it. I also don't expect reviewers to be able to spot code that isn't black-compliant. If you think of checking it or asking about it in a review, great. Otherwise, I don't think we should be too afraid of errors slipping in, since they're trivial to fix. But to be frank, as a reviewer today, I don't really know what to check in the Python code formatting. Our wiki: https://sourceware.org/gdb/wiki/Internals%20GDB-Python-Coding-Standards says that we follow PEP8. I have no idea how to format PEP8 code, so as a result I'm not really checking the formatting of Python code today. > 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. Indeed, I should have mentioned this: we would define a version to use, just like we do for autoconf/automake. The goal of black is that the output won't change just for fun, but it could change due to fixed bugs. > 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. This is why I think it's easier to start from a clean state, by formatting everything in one shot in the beginning. If the issue you describe happens while I am writing a patch, I would notice the spurious diff and push an obvious "Format foo.py with black" commit and rebase my WIP patch on top. If the issue you describe happens while I am revieweing a patch, I would probably also push an obvious re-formatting commit (or ask the person to do so, if they have push access). The offending hunk will go away when they rebase, which is necessary prior to pushing. > 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. Indeed. I hope I was able to show that these situations can be handled easily and are not a showstopper. >> >> 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. Even in corporate environment, you can't assume that! > 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, Simon