Mail Archives: geda-user/2021/04/25/16:29:50
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
- Raw text -