X-Authentication-Warning: delorie.com: mail set sender to geda-user-bounces using -f
X-Recipient: geda-user AT delorie DOT com
Date: Sun, 25 Apr 2021 22:28:47 +0200 (CEST)
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] xspice integration
In-Reply-To: <07d13043-cd66-88f1-4b6c-172345c864e1@epilitimus.com>
Message-ID: <alpine.DEB.2.21.2104252228270.2187@nimbus>
References: <b609b43e-9209-3809-ef0e-d3b1e4b11c3c AT epilitimus DOT com> <alpine DOT DEB DOT 2 DOT 21 DOT 2104142041100 DOT 7753 AT nimbus> <9fe0dd9a-cbb8-a51f-f63d-36cd6d3a31c7 AT epilitimus DOT com> <alpine DOT DEB DOT 2 DOT 21 DOT 2104172241210 DOT 6430 AT nimbus> <alpine DOT DEB DOT 2 DOT 21 DOT 2104231908590 DOT 5702 AT nimbus>
 <4775a561-41ad-5368-271a-998ded5bfbc6 AT epilitimus DOT com> <alpine DOT DEB DOT 2 DOT 21 DOT 2104232352150 DOT 14831 AT nimbus> <e2ab51e4-29df-35e0-e16b-4f48dcbc81c8 AT epilitimus DOT com> <alpine DOT DEB DOT 2 DOT 21 DOT 2104241511400 DOT 1853 AT nimbus> <07d13043-cd66-88f1-4b6c-172345c864e1 AT epilitimus DOT com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII; format=flowed
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

On Sat, 24 Apr 2021, Glenn (glimrick AT epilitimus DOT com) [via 
geda-user AT delorie DOT com] wrote:
> As for the repo problem I had forgotten to set up to automatically do
> update-server-info after each push. I fixed that so you should now get
> the current version.

Huh.  So what I was looking at doesn't really have much in common with 
your latest version...

While I'm quite in favor of adding the xspice backend to gEDA/gaf and 
willing to provide the necessary infrastructure from the gnetlist side of 
things, the problem with this backend (as well as your SAB branch) is that 
it's not a series of targeted patches but basically a bunch of code that 
isn't integrated with gEDA/gaf at all.  This makes it MUCH harder to 
review to a point where it becomes a chore.

To help me explain what I mean, have a look at your patch for the gaf 
export "--layout" option (commit 61fd41ea):

>  static struct option export_long_options[] = {
> -  {"no-color", 0, NULL, 2},
> -  {"align", 1, NULL, 'a'},
> -  {"color", 0, NULL, 'c'},
> -  {"dpi", 1, NULL, 'd'},
> -  {"format", 1, NULL, 'f'},
> -  {"font", 1, NULL, 'F'},
> -  {"help", 0, NULL, 'h'},
> -  {"layout", 0, NULL, 'l'},
> -  {"margins", 1, NULL, 'm'},
> -  {"output", 1, NULL, 'o'},
> -  {"paper", 1, NULL, 'p'},
> -  {"size", 1, NULL, 's'},
> -  {"scale", 1, NULL, 'k'},
> +  {"no-color", no_argument, NULL, 2},
> +  {"align", required_argument, NULL, 'a'},
> +  {"color", no_argument, NULL, 'c'},
> +  {"dpi", required_argument, NULL, 'd'},
> +  {"format", required_argument, NULL, 'f'},
> +  {"font", required_argument, NULL, 'F'},
> +  {"help", no_argument, NULL, 'h'},
> +  {"layout", required_argument, NULL, 'l'},
> +  {"margins", required_argument, NULL, 'm'},
> +  {"output", required_argument, NULL, 'o'},
> +  {"paper", required_argument, NULL, 'p'},
> +  {"size", required_argument, NULL, 's'},
> +  {"scale", required_argument, NULL, 'k'},
>    {NULL, 0, NULL, 0},
>  };

What is happening here?  Only if you know what you are looking for (or 
have an exceptionally keen eye), you will spot that in the "layout" line, 
"0" was not replaced with "no_argument" but with "required_argument". 
This is the actual fix; anything else doesn't change gaf's behavior.

If you think gaf should be using constants here instead of plain integers 
(which I agree with), please change that in a separate commit.  Since this 
is a bugfix that is supposed to go into the stable branch, the commit 
changing the "0" to a "1" should go first (I already did that, see commit 
2077aa58), and the one replacing the integers with equivalent constants 
should go second.  This way, I can apply the bugfix to "stable" and both 
commits to "master".

As a general rule, each change you submit should be self-contained, and it 
should be completely self-evident that the only sane option for me is to 
merge it.  If you see something that may be a problem: fix it.  For 
example, I found the following line in your code:

> librarys = [] #libraries to be included (yes I know it isn't spelled right)

I'm completely at a loss here.  If you realized that you spelled it wrong, 
why didn't you fix it?  If there is a reason not to fix it (as in, it's 
spelled that way in the standard), why didn't you document that so the 
next person to touch the code doesn't "fix" it as the first thing they do?

If you think I'm being too harsh, have a look at the contribution 
guidelines of other projects:

     https://git-scm.com/docs/SubmittingPatches

When contributing myself, I'm following the same principles, and I'm 
putting patches in a form just as if to pass an imaginary review.  You can 
scroll back in the Git history to a practically arbitrary point and will 
see small, complete patches that target a single issue.  Sometimes, 
commits are quite large (like commit 49637284 which adds the complete 
class GschemDockable), but that's in line with the principles: the commit 
history shouldn't reflect your edit history, and if you have one big 
commit that adds the bulk of, let's say, the xspice backend, that's 
completely fine.  Just make sure you split preparatory changes to existing 
code, whitespace changes and the like out into separate commits.

The same goes for coding style.  It shouldn't be too hard to run a regexp 
over your files which adds a space before and after an operator, or break 
overlong lines.  Keep in mind that I, too, am puting a lot of hours of 
good working time into this.  The number of round-trips we'll manage 
before one of us has to go off to solve other problems is limited; you 
won't want to waste these on formatting issues.

> I am actually unclear as to what exactly the license is that geda/gaf is 
> released under as the COPYING file references GPL and there is also a 
> COPYING.LGPL file.

That's an oversight on my part, sorry.  With the possible exception of 
gschem/src/gschem_accel_label.c (I'm not really sure what to do about that 
file), gEDA/gaf is under the GPL, version 2 or later.  Some files (mostly 
the wiki) are under the GFDL.

Roland