From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp1 ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms11 with LMTPS id 0N+sB/+g8V/pWAAA0tVLHw (envelope-from ) for ; Sun, 03 Jan 2021 10:48:31 +0000 Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp1 with LMTPS id GNNcA/+g8V9EQwAAbx9fmQ (envelope-from ) for ; Sun, 03 Jan 2021 10:48:31 +0000 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id A7F32940355 for ; Sun, 3 Jan 2021 10:48:30 +0000 (UTC) Received: from localhost ([::1]:49920 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kw0w1-00035t-Jl for larch@yhetil.org; Sun, 03 Jan 2021 05:48:29 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:44500) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kw0vs-00035U-AY for guix-devel@gnu.org; Sun, 03 Jan 2021 05:48:20 -0500 Received: from mail-out.m-online.net ([2001:a60:0:28:0:1:25:1]:46208) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kw0vp-00036C-Qr for guix-devel@gnu.org; Sun, 03 Jan 2021 05:48:20 -0500 Received: from frontend01.mail.m-online.net (unknown [192.168.8.182]) by mail-out.m-online.net (Postfix) with ESMTP id 4D7wTT208rz1rtZ9; Sun, 3 Jan 2021 11:48:13 +0100 (CET) Received: from localhost (dynscan1.mnet-online.de [192.168.6.70]) by mail.m-online.net (Postfix) with ESMTP id 4D7wTT1LdQz1qs65; Sun, 3 Jan 2021 11:48:13 +0100 (CET) X-Virus-Scanned: amavisd-new at mnet-online.de Received: from mail.mnet-online.de ([192.168.8.182]) by localhost (dynscan1.mail.m-online.net [192.168.6.70]) (amavisd-new, port 10024) with ESMTP id BAPo2x65lu3d; Sun, 3 Jan 2021 11:48:11 +0100 (CET) Received: from hermia.goebel-consult.de (ppp-188-174-49-60.dynamic.mnet-online.de [188.174.49.60]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.mnet-online.de (Postfix) with ESMTPS; Sun, 3 Jan 2021 11:48:11 +0100 (CET) Received: from lenashee.goebel-consult.de (lenashee.goebel-consult.de [192.168.110.2]) by hermia.goebel-consult.de (Postfix) with ESMTP id B45EF60213; Sun, 3 Jan 2021 11:53:38 +0100 (CET) From: Hartmut Goebel Subject: Re: [RFC] Improve Python package quality To: Lars-Dominik Braun , guix-devel References: Organization: crazy-compilers.com Message-ID: <11b7fa96-1ae8-bc1f-6c11-58f4abfb57b7@crazy-compilers.com> Date: Sun, 3 Jan 2021 11:48:09 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/alternative; boundary="------------7FDF9B6754B92579F4749365" Content-Language: en-US Received-SPF: none client-ip=2001:a60:0:28:0:1:25:1; envelope-from=h.goebel@crazy-compilers.com; helo=mail-out.m-online.net X-Spam_score_int: -36 X-Spam_score: -3.7 X-Spam_bar: --- X-Spam_report: (-3.7 / 5.0 requ) BAYES_00=-1.9, HTML_MESSAGE=0.001, NICE_REPLY_A=-1.118, RCVD_IN_DNSWL_LOW=-0.7, SPF_HELO_NONE=0.001, SPF_NONE=0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: guix-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+larch=yhetil.org@gnu.org Sender: "Guix-devel" X-Migadu-Flow: FLOW_IN X-Migadu-Spam-Score: -2.33 Authentication-Results: aspmx1.migadu.com; dkim=none; dmarc=none; spf=pass (aspmx1.migadu.com: domain of guix-devel-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=guix-devel-bounces@gnu.org X-Migadu-Queue-Id: A7F32940355 X-Spam-Score: -2.33 X-Migadu-Scanner: scn0.migadu.com X-TUID: /k4b8zZGUChF This is a multi-part message in MIME format. --------------7FDF9B6754B92579F4749365 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Hi Lars, this is a good idea. (Since you where mentioning setuptools, I first was afraid your solution would be tightened to setuptools, but it is not. Well done!) Some comments (most of which are nit-picking): > +;; Python 2 support. > > +"from __future__ import print_function" > This comment should go behind the line of code, as it only related to that single line. > +" req = str (dist.as_requirement ())" > > +;; dist.activate() is not enough to actually check requirements, we > have to > > +;; .require() it. > > +" pkg_resources.require (req)" > I suggest putting the comments into the python source. This would allow to indent them according the the python code, which would make it easier to understand. This would also allow to use a single multi-line guile-string, which allows to easiyl copy the script out and in from the guile-source for testing it. > +" except Exception as e:" > > +" print (req, e)" > > +" sys.exit (1)" > raise SystemExit(1) > +" print ('...trying to load endpoint', group, name)" Please follow PEP8 (no space before opening parentheses) - also at other places. > +" for name in dist.get_metadata_lines ('top_level.txt'):" > > +" print ('...trying to load module', name)" > Add `end=""`, thus the "result" can be printed on the same line. > +" except ModuleNotFoundError:" > > +" print ('......WARNING: module', name, 'not found, continuing')" > Print result terse, on same line, without repeating the name:   print (' WARNING: not found') > +" continue") > Add printing success:     else:        print("passed") > + (add-installed-pythonpath inputs outputs) > > + ;; Make sure the working directory is empty (i.e. no Python modules > in it) > > + (with-directory-excursion "/tmp" > Would is be better to use mkdtemp here to ge a fresh, empty directory? -- Regards Hartmut Goebel | Hartmut Goebel | h.goebel@crazy-compilers.com | | www.crazy-compilers.com | compilers which you thought are impossible | --------------7FDF9B6754B92579F4749365 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit
Hi Lars,

this is a good idea. (Since you where mentioning setuptools, I first was afraid your solution would be tightened to setuptools, but it is not. Well done!)

Some comments (most of which are nit-picking):

+;; Python 2 support.

+"from __future__ import print_function"

This comment should go behind the line of code, as it only related to that single line.

+" req = str (dist.as_requirement ())"

+;; dist.activate() is not enough to actually check requirements, we have to

+;; .require() it.

+" pkg_resources.require (req)"

I suggest putting the comments into the python source. This would allow to indent them according the the python code, which would make it easier to understand. This would also allow to use a single multi-line guile-string, which allows to easiyl copy the script out and in from the guile-source for testing it.


+" except Exception as e:"

+" print (req, e)"

+" sys.exit (1)"

raise SystemExit(1)


+" print ('...trying to load endpoint', group, name)"

Please follow PEP8 (no space before opening parentheses) - also at other places.


+" for name in dist.get_metadata_lines ('top_level.txt'):"

+" print ('...trying to load module', name)"

Add `end=""`, thus the "result" can be printed on the same line.

+" except ModuleNotFoundError:"

+" print ('......WARNING: module', name, 'not found, continuing')"

Print result terse, on same line, without repeating the name:

  print (' WARNING: not found')

+" continue")

Add printing success:

    else:
       print("passed")

+ (add-installed-pythonpath inputs outputs)

+ ;; Make sure the working directory is empty (i.e. no Python modules in it)

+ (with-directory-excursion "/tmp"

Would is be better to use mkdtemp here to ge a fresh, empty directory?


-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |
--------------7FDF9B6754B92579F4749365--