X-Authentication-Warning:	delorie.com: mail set sender to geda-user-bounces using -f
X-Recipient:	geda-user AT delorie DOT com
Date:	Fri, 23 Oct 2020 15:43:50 +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] submitted a new patch
In-Reply-To:	<aa7c0456-a606-be86-58c4-b8352cc66127@epilitimus.com>
Message-ID:	<alpine.DEB.2.21.2010231526280.12318@nimbus>
References:	<14f9e862-8ee0-4432-23b6-06e94215baa4 AT epilitimus DOT com> <alpine DOT DEB DOT 2 DOT 21 DOT 2010120958150 DOT 2535 AT nimbus> <32bfe083-3604-b747-030a-48a13e2b1074 AT epilitimus DOT com> <alpine DOT DEB DOT 2 DOT 21 DOT 2010122312420 DOT 8245 AT nimbus> <7c133ba2-5b09-91f3-808f-9f444c625278 AT epilitimus DOT com>
	<alpine DOT DEB DOT 2 DOT 21 DOT 2010151343310 DOT 1527 AT nimbus> <aa7c0456-a606-be86-58c4-b8352cc66127 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

Hi Glenn,

I had a look at your patch.  First, a few formal things:

- The preferred way for patch submissions is a patch generated by Git 
(e.g. by using "git commit" and then "git format-patch HEAD^..HEAD").  I 
had to apply your patch by hand, which was doable because it's a small 
patch but quickly gets impractical for larger patches.

- The Python patch looks good.

- The Guile patch was not against current Git master (it contained an 
extra debug message which I had removed in a subsequent commit).  Again, 
this is not a big issue because the patch is small, but it's best practice 
to always rebase your patch onto the latest commit.

- The Guile patch contains unnecessary changes (adding a newline character 
to an unrelated debug message; removing an unrelated comment).  Patches 
should not contain any changes which are not immediately relevant to the 
issue the patch addresses.  If you think these changes would improve the 
project, please submit them as an individual patch.

The change itself looks good.  I don't know enough about simulation to 
judge whether a parameter for switching between "blah blah" and ".TITLE 
blah blah" as the first line of the output makes sense, but it is 
implemented in the straightforward way.  What bothers me is the usage of 
the "comment=" attribute: to me, specifying options in an attribute named 
"comment=" appears to be bad design practice.  Then again, I don't know 
much about simulation, so "comment=" may be the obvious place to put this.

Any opinions from people who use SPICE?

Roland

- Raw text -