delorie.com/archives/browse.cgi   search  
Mail Archives: geda-user/2020/12/01/10:42:01

X-Authentication-Warning: delorie.com: mail set sender to geda-user-bounces using -f
X-Recipient: geda-user AT delorie DOT com
Date: Tue, 1 Dec 2020 16:22:15 +0100 (CET)
From: Roland Lutz <rlutz AT hedmen DOT org>
To: "Glenn (glimrick AT epilitimus DOT com) [via geda-user AT delorie DOT com]" <geda-user AT delorie DOT com>
Subject: Re: [geda-user] SAB processing patches
In-Reply-To: <7c75ed03-456c-b408-8b50-0448f6b3a04f@epilitimus.com>
Message-ID: <alpine.DEB.2.21.2012011606400.6785@nimbus>
References: <e4ff3c96-939b-a93e-a32f-5e938b6daa63 AT epilitimus DOT com> <alpine DOT DEB DOT 2 DOT 21 DOT 2011302045040 DOT 2894 AT nimbus> <20201130220505 DOT 0AE4282C54FD AT turkos DOT aspodata DOT se> <7c75ed03-456c-b408-8b50-0448f6b3a04f AT epilitimus DOT com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Reply-To: geda-user AT delorie DOT com
Errors-To: nobody AT delorie DOT com
X-Mailing-List: geda-user AT delorie DOT com
X-Unsubscribes-To: listserv AT delorie DOT com

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323329-1194489676-1606836135=:6785
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8BIT

Hi Glenn,

On Mon, 30 Nov 2020, Glenn (glimrick AT epilitimus DOT com) [via 
geda-user AT delorie DOT com] wrote:
> Old habits die hard :)

no problem. ;-)  It took me some time to get adjusted to the gEDA 
conventions, as well.  Especially the spaces in front of argument lists…

I have fixed the remaining formatting issues as I encountered them:

     https://github.com/rlutz/geda-gaf/commits/glenn-sab

So here's the next batch of feedback.  I still haven't got to the core of 
it (like "does this make sense" and "is this implemented in a way that's 
copasetic with the rest of gEDA/gaf"), but I'll get there eventually.


- Most things in gEDA/gaf are case-sensitive, so it makes sense to treat
   SAB arguments as case-sensitive, as well.

- getlist isn't designed for backends to change component/package names,
   much less blueprint names.  This may work, or it may not.  If you need
   to rename things, it's better to use a filtered component/package list
   instead (see the SPICE backends for an example).

- You don't need "nobase_" on your "example_DATA" target (you don't have
   files in subdirectories), but it doesn't hurt, either.

- Fixes to existing code (like the memory leak in s_slot.c) should go
   into a separate patch.


- Header files (especially public ones) should only include those other
   header files which they minimally require in order to work.  AFAICT,
   for "sab.h", this would be just "struct.h".

- Don't assign variables inside function calls unless you have a good
   reason to do so.  Assigning from "strtok" inside "while" in sab.c is one
   of the few cases where this is ok (as the assignment can't be put on a
   separate line without hurting readability); most other cases are not.

- "TOPLEVEL *" variables are called "toplevel" (all lower-case) in the
   existing code, so it's a good idea to follow this convention for new
   code, as well.


- gEDA/gaf Python code should reside within the "gaf" top-level package.
   You basically have two choices for organizing the SAB code: either
   (1) have individual sub-modules of gaf by placing the code files in
       "xorn/src/python/", then import "gaf.sab" and call
       "gaf.sab.process", or
   (2) group the SAB source as a sub-package by placing the code in
       "xorn/src/python/sab/", then import "gaf.sab.sab" and call
       "gaf.sab.sab.process"
   I'd go with the first option, but either way is okay.

- Don't re-define "process" in "sab/__init__.py".  There is some point
   in doing this for overloaded or publicly-visible functions, but here
   it just confuses things.

- You are processing the "--sab-context" command-line argument in
   "netlist.py", so it is not necessary to process it in "gnetlist.in",
   as well.  You can just pass it through unchanged.

- User-visible messages should be wrapped in _("...") in order to be
   translatable.

- Since you use the tuple (order, action, parms) in multiple places, it
   may be worth defining a collections.namedtuple for that purpose.

- Don't use something like 1000000000 as a special value.  If you have
   to store the absence of a value in-band, use something that cannot
   ever occur as a valid value, like -1 or None.  (And if you *really*
   have to use a magic value somewhere, please define it as a constant.)

- In modify_refdes.py, the variable "refdes" is either a list or a
   string.  Since the expression "'C' in ..." works in both cases, I
   suggest replacing "list(levels.upper())" with "levels.upper()".

- You don't need types.StringType and types.ListType;
   "isinstance(..., str)" and "isinstance(..., list)" work just fine.

- Using "dir(m)" to determine whether a variable is defined in a module
   feels a bit too hacky.  I'm not sure yet how I'd prefer this to be
   solved; if you absolutely have to make the code depend on whether or
   not there is such a variable, use it and catch an AttributeError.

- Instead of:      ''.zfill(4 * indent).replace('0', ' ')
   you can just do: '    ' * indent

- Bonus points for using str.partition!  I didn't know that one.


Roland

--8323329-1194489676-1606836135=:6785--

- Raw text -


  webmaster     delorie software   privacy  
  Copyright © 2019   by DJ Delorie     Updated Jul 2019