rougier / persid

Persistent identifier library for GNU Emacs
GNU General Public License v3.0
35 stars 3 forks source link

Use OpenLibrary Book API instead of ebooks.de #6

Closed fabcontigiani closed 6 months ago

fabcontigiani commented 7 months ago

Hi Nicolas! First, I wanted to thank you for creating this package and for your contributions to the Emacs community in general! Big fan of your work ^^

OpenLibrary was suggested on #4 as an alternative source for retrieving bibliographic entries based on ISBN. Recently I dabbled with one of OpenLibrary's APIs (biblio-openlibrary), and wanted to contribute to this package too, since it may fit better in the workflow I wanna implement with the built-in bibtex-mode and citar.

This PR uses the Book API from OpenLibrary to receive a response in JSON form given a valid ISBN identifier, then parses it extracting the necessary information to create a BibTeX entry of type 'Book' from scratch, due to this previously being done on the server side by ebooks.de.

I decided to use OpenLibrary's "Legacy" Book API, as indicated on their documentation, seeing that it was possible to receive all needed information in a single query, in contrast, when using other APIs, e.g. the Search API, it was needed to perform an additional query for every author, since only their IDs is received. In addition, OpenLibrary's documentation only mentions the posibility of the Book API being phased out in the future, rather than it being a certainty.

When compiling the package with the proposed changes I get an error, I'm still a noob when it comes to elisp and I'm not sure if and how I've introduced it:

Error (bytecomp): ‘add-to-list’ can’t use lexical var ‘formats’; use ‘push’ or ‘cl-pushnew’

Executing package-install-file on persid.el gives some more information:

Compiling file /home/fab/.config/emacs/elpa/persid-0.1.0/persid.el at Mon Mar 25 19:57:00 2024
Entering directory ‘/home/fab/.config/emacs/elpa/persid-0.1.0/’
persid.el:98:12: Warning: defcustom for ‘persid-mail-address’ fails to specify
    type
persid.el:98:12: Warning: defcustom for ‘persid-mail-address’ fails to specify
    containing group

In persid-identify:
persid.el:345:47: Error: ‘add-to-list’ can’t use lexical var ‘formats’; use
    ‘push’ or ‘cl-pushnew’

If it helps this is how I've installed and configured it in my init file:

(use-package persid
  :vc (:fetcher github :repo fabcontigiani/persid))

~Doing M-x persid-bibtex-from and providing a valid ISBN identifier doesn't work, nor does it throw an error for me at the moment, it does nothing really.~ Nevertheless, when evaluating (insert (persid-bibtex-from-isbn "some-valid-isbn")) the BibTeX entry gets inserted correctly, for example:

(insert (persid-bibtex-from-isbn "9781491952962")) ;; C-x C-e

I get: (might need to add a new line jump or something, not sure)

(insert (persid-bibtex-from-isbn "9781491952962"))@book{,
title     = {Practical Statistics for Data Scientists},
author    = {Peter Bruce and Andrew Bruce and Peter Gedeck},
publisher = {O’Reilly Media},
year      = {2017},
isbn      = {9781491952962},
url       = {http://openlibrary.org/books/OL26836530M/Practical_Statistics_for_Data_Scientists},
}

Notice the absence of a citekey, the creation of the BibTeX entry was done on the server side by ebooks.de previously, thus a citekey was generated there. As it stands now the generated BibTeX lacks this feature, but it allows the built-in bibtex-mode to autogenerate one for the user with the available information on the entries by pressing C-c C-c, if the bibtex major mode is active that is, this has the upside of respecting the user's configuration of the bibtex package making the generation of the citekey customizable, otherwise is up to the user to write one. It may be possible to automatically generate a key using the different tools made available with bibtex.el but it would add a dependency and complexity, which I'm not sure if it's desirable.

Also, I've added biblio-openlibrary to the related works mentioned on the readme, let me know if you feel it doesn't belong there and I'll remove it, no worries.

As I said, I'm still quite new to elisp and all feedback is well received. Cheers!

fabcontigiani commented 7 months ago

Decided to add an option for automatic citekey generation since it was simpler than I anticipated, I'm not sure how to indicate the new dependency (bibtex-mode), maybe it isn't necessary since it's built into Emacs.

fabcontigiani commented 7 months ago

I was under the impression that persid-bibtex-from would insert text into the buffer when called with M-x. However, after reading #5, it is clear that this is not the case. I still need to do some more testing, but this should be working properly.

fabcontigiani commented 7 months ago

Sorry, made a mess trying to rename branches 😅

rougier commented 6 months ago

Thanks for this amazing PR. To answer your first question on lexical vars, I'm not too sure about the reason but it might related to a non existent local variable. I think we can deal with that later.