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 To: "Glenn (glimrick AT epilitimus DOT com) [via geda-user AT delorie DOT com]" Subject: Re: [geda-user] submitted a new patch In-Reply-To: Message-ID: References: <14f9e862-8ee0-4432-23b6-06e94215baa4 AT epilitimus DOT com> <32bfe083-3604-b747-030a-48a13e2b1074 AT epilitimus DOT com> <7c133ba2-5b09-91f3-808f-9f444c625278 AT epilitimus DOT com> User-Agent: Alpine 2.21 (DEB 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; format=flowed; charset=US-ASCII 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 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