Re: [ELPA] New package: calibre.el

["Kjartan Óli Águstsson" <kjartanoli@outlook.com>]

Thank you for taking the time to look at it.

>>                                                 Would there be any
>> interest in adding it to GNU ELPA? And if so, is my copyright assignment
>> for Emacs sufficient, or does ELPA require a separate assignment?
>
> No separate assignment is necessary, as GNU ELPA packages are regarded
> to be part of Emacs.

Good to know.

>> This is my first attempt at writing an Emacs package, so I expect to
>> have gotten many things wrong.  As such I would welcome reviews from
>> people who know more about Elisp packaging.
>
> The first thing to note is that you don't need a -pkg.el file.  ELPA
> will generate one for you using the metadata in the main file and
> overwrite whatever you have written.

Another good to know.  I remember reading that somewhere, but then I
looked at some other packages that seemed to maintain a -pkg.el file.

> This means you should copy the metadata to calibre.el.  Especially the
> dependency list.  (Also, why do you depend on "29.1.0", a version which
> is unreleased and has an additional ".0" at the end?  I guess you need
> Emacs 29 because of SQLite?  Have you taken a look at emacsql?)

I'll definitely fix the .0 thing.  You are correct that the dependency
on Emacs 29 is for SQLite.  Emacsql would not work, since I am
interacting with an existing database maintained by Calibre.  If you
want to wait until Emacs 29 is released to add it I would definitely
agree to that.

> From a brief skim of the code, it looks more or less fine.  There are
> minor things I am not sure about (such as the usage of eieio or why you
> declare some functions instead of requiring the file).

The functions that are declared instead of required currently cause a
recursive require because of how the package is structured.  I am hoping
to refactor this soon.

As for the usage of eieio, is there a reason not to use it?

-- 
Kjartan Oli Agustsson
GPG Key fingerprint: 4801 0D71 49C0 1DD6 E5FD  6AC9 D757 2FE3 605E E6B0