Mail Archives: geda-user/2020/12/01/10:42:01
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 -