Hey Kjartan, nice to see you writing packages and contributing to Emacs now.

This sounds interesting indeed, but it would be nice if you could add some
(more) explanation about the
 package in the repo its README and/or in the initial comment of the
calibre.el file.

Also, I am using the very mature calibredb.el
<https://github.com/chenyanming/calibredb.el> package for a few years
already. Are you already aware
of its existence? If so, besides that that package is only available from
Melpa, could you explain why you
wrote this similar alternative package?

Anyway, thanks for sharing your package! And nice job!

On Tue, 18 Apr 2023 at 10:36, Kjartan Óli Águstsson <kjartanoli@outlook.com>
wrote:

> 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
>
>