caldwell / commit-patch

Commit patches to Darcs, Git, Mercurial, Bazaar, Monotone, Subversion, or CVS
http://www.porkrind.org/commit-patch/
GNU General Public License v2.0
21 stars 8 forks source link

follow header conventions #3

Closed tarsius closed 12 years ago

tarsius commented 12 years ago

I maintain the Emacsmirror a mirror of Emacs packages here at github.

Eventually I want to provide a package manager which uses this mirror. This package manager is far from ready but it can already display a list of available packages and details about a package; similar to package-list-packages and describe-package from the official (as of the to be released Emacs-24) package.el package manager.

Since I mirror more than 3000 packages I can not maintain the package metadata manually like the maintainers of package.el and el-get.el do. Instead I extract it using the builtin library lisp-mnt.el and some custom tools.

Unfortunately many maintainers do not follow these conventions at all or not close enough for these extraction tools to work properly.

I mirror about 800 packages from github among which 120 do not follow the most important convention: how the summary line has to be formatted.

;;; foo.el --- Do foo with bar

For these 120 packages I am currently creating patches and submitting pull requests like this one.

In some cases I also corrected some other problems concerning the library header and/or deleted trailing whitespace. If I did not fix anything but the summary line please don't assume that the rest of the header is 100% correct; you might still want to improve it.

Also note that if you maintain additional packages I might not have created such a pull request for them even when they fail to follow the header conventions closely enough. I do so only (at this point) if the summary line can not be parsed. So please fix the headers of other libraries yourself.

Please read the official Header Conventions. Note that while creating this patch I tried to preserve your personal header style while following these conventions as closely as required by the extraction tools. You might want to further improve the header.

Here are some additional notes.

(To save some time I created this generic pull request message which mentions some common problems which might not all apply to this package. So please forgive me if some of this is not relevant to you.)

Summary Line

;;; foo.el --- Do foo with bar

This has to be on the first line, there are three semicolons and three dashes. The library name has to match the filename (therefor it ends with .el) and the provided feature (which obviously doesn't end with .el tough).

If you also define local variables using -*- ... -*- then that has to be done on the next line or at the end of the first line.

;;; foo.el --- Do foo with bar -*- baz: t -*-

or

;;; foo.el --- Do foo with bar
;; -*- baz: t -*-

There is not reason for -*- mode: emacs-lisp -*-. Really Emacs detects that on it's own... Also it is usually not necessary to specify the coding-system.

If the summary is rather long do not split it into two lines, the extraction tools fail to parse that. Instead make them shorter, which is also useful when the summary is displayed when listing packages or elsewhere.

Don't worry if the summary line is longer than 74 or 80 characters; what matters is the length of the summary sans the leading filename. It's probably a good idea for the summary to be no longer than ~30-40 characters (or even less) - but again if you can't make it this short without losing essential information don't worry to much.

However this:

;;; foo.el --- Foo is a global minor mode to do foo repeatedly while also going to the local bar based on baz

can probably be abbreviated as:

;;; foo.el --- Do foo with bar

Move the longer description to the Commentary section.

Also please make the summary useful to people who don't have the necessary domain knowledge. E.g. they should be able to tell from the summary that the package implements a major mode for some obscure programming language they have never heard of before and therefor is not of interest to them.

Commentary Section

;;; Commentary:

This begins with three dashes and ends with a colon.

The first sentence or paragraph should repeat the information from the summary line. Try to be brief but use whole sentences - this isn't the summary line, we have enough room.

If you feel the need to provide installation instructions preferably do so toward the end of the commentary section.

You might even want to create a separate section for the installation instructions. This breaks with the header conventions (or at least isn't mentioned there) but has the advantage that tools that extract the commentary won't include the installation instructions. And that is useful because these tools are commonly used to extract information for the benefit of a package manager, so the generic instructions would actually be incorrect because installation happens with something like M-x pm-install-package.

;;; Commentary:

;; This package...

;;; Installation:

;; Put `foo.el' in the `load-path' and add
;;
;;   (foo-mode)
;;
;; to your init file.

In any case do not only provide the installation instructions. At the very least duplicate the summary line at the beginning of the commentary section.

Also if your installation instructions are as generic as in the example above consider dropping them completely. This is common knowledge and for a complete beginner who never installed a package before yours (how likely is that?) this isn't enough information anyway (what is that load-path, where is my init file?). Don't duplicate the generic installation instructions from the Emacs info page in an incomplete way - just inform about additional things to consider.

Code Section

Put ;;; Code: after the commentary section and before the first require form. This might be kind of pointless but not following this convention just because is also rather pointless. (By the way lm-commentary will fail without this but my variant of that function does not.)

;;; Commentary:

;; This package...

;;; Code:

(require 'bar)
...

Maintainer and Author Header Lines

The convention requests both to be specified. In my opinion you can omit Maintainer if it is equal to Author.

;; Author: Jonas Bernoulli <jonas@bernoul.li>
;; Maintainer: Jonas Bernoulli <jonas@bernoul.li>

Multiple authors can be specified but only one maintainer. Note that you can't use Authors (plural) when there are multiple authors - lm-authors won't detect it (though my variant does).

;; Author: Jonas Bernoulli <jonas@bernoul.li>
;;      Linus Bernoulli <linus@bernoulli.cc>
  [^tab] (only one tab, nothing else)

Please consider not obfuscation the email address and wrap it with <...> even if you do obfuscate it.

Footer Line

(provide 'foo)
;;; foo.el ends here

Don't forget to provide the appropriate feature. The ;;; foo.el ends here? Omit it if you want I don't care. I think this is rather pointless in the age of distributed version control, then again not everybody has arrived there yet. I don't omit it in my libraries - that would weaken my position when telling people that following the conventions is important :-)

tarsius commented 12 years ago

ping (pull request is 4 months old now)

tarsius commented 12 years ago

pong

tarsius commented 12 years ago

(Don't just merge. Replace the ??? in the summary with an actual summary. (That's kinda obvious but some people actually just merged similar pull requests.))

tarsius commented 12 years ago

Closing pull request to free my list and removing from the emacsmirror because it is has to be considered unmaintained.

caldwell commented 12 years ago

removing from the emacsmirror

That's actually probably a good idea anyway since commit-patch-buffer.el really isn't useful on its own. It requires commit-patch to be installed and has no use outside the commit-patch package.

has to be considered unmaintained.

:-) Just because I'm going out of my way to do these header changes doesn't mean it's unmaintained, but thanks for the condescension.

My plan (that I technically haven't communicated) is to put the headers in next time I worked on commit-patch-buffer.el. The code is fairly stable and I haven't had to touch it in quite a while.

-David

tarsius commented 12 years ago

:-) Just because I'm going [not] out of my way to do these header changes doesn't mean it's unmaintained, but thanks for the condescension.

I didn't say it is unmaintained just that I consider it so. I do so because - due to the lack of any response at all - I cannot justify putting any more work into getting this problem fixed that I could instead use for more productive work on the Emacsmirror. Also I want it of my todo list: why should I care enough to keep it there if the maintainer doesn't care enough to reply?

Also there are several other reported issues regarding this package and they are from actual users of your packages that reported real problems and in each case you did not respond at all even though these reports are also several months old. Since the only person who could tell me whether the package is actually still maintained doesn't respond to communication attempts I have to resort to observations like this and they all point in one direction.

You also have to admit that word that offened you did it's job, exactly because it did: you actually responded.

caldwell commented 12 years ago

Also I want it of my todo list: why should I care enough to keep it there if the maintainer doesn't care enough to reply?

That's fair enough, I don't begrudge you that.

Also there are several other reported issues regarding this package and they are from actual users of your packages that reported real problems and in each case you did not respond at all even though these reports are also several months old.

Links please. I'm unaware of any reported issues that have not been responded to. Most issues to commit-patch come through email. The one single github issue has a comment from me on it and is currently waiting for a release in an upstream dependency. If there are issues elsewhere they are unofficial and I can't possibly be expected to know about them.

I didn't say it is unmaintained just that I consider it so.

Hmmm, so if I were to say I considered you an asshole, that's cool?

I'm not saying that, by the way, but it stung for a second, right? The point is that trying to hide behind weasel words like that doesn't really change anything.

You also have to admit that word that offened you did it's job, exactly because it did: you actually responded.

Yes, congratulations, but the exchange has left a very bad taste in my mouth and I probably will avoid adding the stupid comments at all (since I am a horrible, spiteful person).

You do realize that you were pestering me about (and now we're arguing about) comments, right?

-David

tarsius commented 12 years ago

Links please.

I didn't look very closely, maybe you responded elsewhere and just forgot to close the issues.

I didn't say it is unmaintained just that I consider it so.

Hmmm, so if I were to say I considered you an asshole, that's cool?

No, it's not. Also that sentence might be read as implying just that. It's also not quite the same - both because one targets a person and the other a software project and because I still do think that the "consider" actually does make a difference in my sentence.

I'm not saying that, by the way, but it stung for a second, right?

After I looked it up "condescension" already did.

I didn't say it is unmaintained just that I consider it so.

The point is that trying to hide behind weasel words like that doesn't really change anything.

The "consider" was in the original message, so I am not resorting to it now to change the meaning of what I said initially (I don't think that's what you are saying just pointing it out). Also you are taking the above sentence out of context as I go on (not very well) to explain why I do so.

I can understand that you don't care that much about this kind of thing; but it would have been nice if you had just said so (that you would eventually get to it) instead of ignoring me. That disappointed my and as a result I used a wording that was intentionally provoking but not meant as the offense, as which it was received.

You also have to admit that word that offened you did it's job, exactly because it did: you actually responded.

Yes, congratulations, but the exchange has left a very bad taste in my mouth

I am not happy about the outcome either. I guess the bad taste comes mainly from the "unmaintained" and the above remark. That remark really wasn't necessary. But those weren't the only things I have said.

and I probably will avoid adding the stupid comments at all

Please don't do that, it doesn't help anyone and also doesn't hurt me. If you don't add them because I don't think they are important that is fine. But not adding to make a point?

(since I am a horrible, spiteful person).

Okay, I am sorry. I really am. You took the "unmaintained" to hard. I admit I used that word to express my disappointment. I did not intend to insult you and actually had hoped (a little) that it might cause a thought process like this: "wow, that guy really cares about this issue if it is enough for him to consider the package unmaintained because of it, well that's a bit silly, but since it is a (small but still) improvement and hardly any work, let's just get it done". Well didn't play out that way.

You do realize that you were pestering me about [comments]

You are not the only one to whom I send such reminders. There were quite a few people who said something along the line: "Sorry, I somehow missed your pull request. Thanks for the reminder." Really. If you had just said that you are not/later going to improve the header, I would have known that you got the message.

(and now we're arguing about) comments, right?

In my original message I have explained why I think this is important and what benefits it has if those comments are added. No problem if you disagree but the reason we are arguing is that "unmaintained" offended you and "condescension" offended me.

Instead of:

removing from the emacsmirror because it is has to be considered unmaintained.

I could have said:

I (the maintainer of the Emacsmirror) do not consider this package maintained well enough for inclusing into the Emacsmirror. I don't know whether it is actually unmaintained or whether the fact that it hasn't seen a commit in over a year is due to it being feature complete and bug free. That isn't very likely and since the only information I could gather is that upstream did not respond to a pull request (and several reminders over the course of several months) which is both trivial and easy to eigher merge or reject. As far as the Emacsmirror is concerned it is therefor considered unmaintained. But it looks promising enough for inclusion into the Emacsattic from which it can moved back in case upstream eventually responds and is willing to eigher merge or explain why not.

Above I said that I had a little hope that my comment would lead to a positive outcome but even more I thought that any response at all, whether positive or negative, was highly unlikely at that point. It just didn't feel like communication. So in a way it did not seem appropriate to be so verbose when the sentence actually used summed it up precisely enough for me to understand it.

The "unmaintained" wasn't meant as a fact. The "consider" should not be considered :-) a weasel word. But I should have said "I consider" instead of "has to be considered". The latter was used to express my frustration for being ignored.

Sincerly, Jonas