unofficial mirror of guix-devel@gnu.org 
 help / color / mirror / code / Atom feed
From: John Darrington <john@darrington.wattle.id.au>
To: Danny Milosavljevic <dannym@scratchpost.org>
Cc: guix-devel@gnu.org
Subject: Re: [PATCH] gnu: Fix load-extension path in packaging of guile-ncurses.
Date: Wed, 21 Dec 2016 10:56:47 +0100	[thread overview]
Message-ID: <20161221095646.GA6917@jocasta.intra> (raw)
In-Reply-To: <20161221093656.400cd7cd@scratchpost.org>

[-- Attachment #1: Type: text/plain, Size: 3710 bytes --]

On Wed, Dec 21, 2016 at 09:36:56AM +0100, Danny Milosavljevic wrote:
     > Sure (I would like to see a convention where such explanations are
     > put in the commit messaage, but I have previously been outvoted on
     > that issue):

Hi Danny, 

A small request: Can you please fold the text of your email to ~80 
characters.  It's very hard to read otherwise.
     
     No, please don't put explanations into the commit message. But do put them into the source code as a comment.

That approach can work sometimes, but more often it is a non-starter.

A few example scenarios:

1. Suppose I need to do a global search and replace, changing a variable name 
across many files.  It would be ludicrous  to have in dozens of files:

;; This variable used to be called "bar" but we changed it to "foo" because ...
(+ foo 4)

When reviewing the code, frankly nobody CARES what it used to be called!


2. Suppose we decide to delete something from a file. It would be equally 
ludicrous to have:

;; There used to be some code here which did:
;;(large-block-of-code
;;.
;;.
;;.
;;.
;;.
;;.)
;; but we decided to delete it because ...

Again I don't care what used to be in a file.


3.  Suppose that, due to a design change, a new variable has to be 
introduced in places thoughout the code:  It would be bizarre, 
distracting and stupid to have in many places:

;; Since we introduced the frobnicator module the signature for
;; calling wiz needs to pass it as an argument.
(wiz frobnicator)



4. Suppose I fix a bug:  It would be pejorative to write:

;; Fred Bloggs who wrote the function typed 'xyz' when he
;; ought to have put 'abc', because ...


I am just glad when a bug has been fixed.  If somebody changes
some code of mine, I might be curious as to why.  In that case
I can check the git log.  If that person has (like he should)
explained in the git log why my code was wrong, I will be 
gratefull for the explanation and the fix.  But nobody except
me will care about bugs in the function which have been fixed.


     I'm also working on other projects, some of which do what you propose. What I often end up having to do there is do git blame, then git log for each line, in order to find out why the source code does what it does. Let's not do that here. 

That is what git blame is for.  Be thankful for it!

  There's a perfectly good inline mechanism for it: Comments.

I am not saying that no explanations of *current* code should 
be put in comments.  It is of course good practice to explain
the working of tricky parts of code.  But to put a *history*
of the code inline is just distraction and a misuse of comments.


Your proposal takes us back to the 1970s - Occassionally I come across
code done like that.  It is  a nightmare to follow.  I am not normally
interested in the history of the code when I look a the source.  I am 
interested in what it does now.   If I want the history, then use git.
That is (amoung other things) what it was designed for.


I hope this explains why putting history in comments is harmful.
Having it in the commit message would certainly have avoided me
having to explain the situation to Mark too.


If this doesn't convince you, then I don't know what more I can say.
But I find that our current git logs are just useless.  They don't
tell me anything which I couldn't have found out by running 
git diff/git blame.

J'


-- 
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

  reply	other threads:[~2016-12-21  9:57 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-12-19 17:50 (unknown), John Darrington
2016-12-19 17:50 ` [PATCH] gnu: Fix load-extension path in packaging of guile-ncurses John Darrington
2016-12-19 21:17   ` Ludovic Courtès
2016-12-20  7:17   ` Mark H Weaver
2016-12-20 11:03     ` John Darrington
2016-12-20 14:16       ` John Darrington
2016-12-21  8:36       ` Danny Milosavljevic
2016-12-21  9:56         ` John Darrington [this message]
2016-12-22  5:56           ` Tobias Geerinckx-Rice
2016-12-22  8:20             ` John Darrington
2016-12-24 15:39               ` Mark H Weaver
2016-12-24 17:02                 ` John Darrington
2017-01-16  0:11                   ` Mark H Weaver

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://guix.gnu.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161221095646.GA6917@jocasta.intra \
    --to=john@darrington.wattle.id.au \
    --cc=dannym@scratchpost.org \
    --cc=guix-devel@gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).