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 To: "Glenn (glimrick AT epilitimus DOT com) [via 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: References: <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 Content-Type: multipart/mixed; boundary="8323329-1194489676-1606836135=:6785" 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 Precedence: bulk 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--