Re: Upstreaming parsebib.el

[Joost Kremers]

On Wed, Jan 29 2025, Philip Kaludercic wrote:
> Joost Kremers <joostkremers@fastmail.fm> writes:
> As for adding the package ELPA: That can certainly be done, could you
> just review these comments and suggestions:

Sure! Some comments below. (I've snipped the suggestions that I have no
questions about. I'll implement those ASAP.)

> @@ -402,7 +402,7 @@ An assignment is the combination of an identifier, an 
> equal sign
>  and a composed value.  A @String definition has exactly one
>  assignment, an entry has a potentially unlimited number."
>    (if-let* ((id (parsebib--identifier))
> -            (_ (parsebib--char '(?=)))
> +            ((parsebib--char '(?=)))

Personally, I find the underscores helpful: it makes it easier for me to
see which clauses are there just for their non-nil value. I understood that
in Emacs 30, flymake will accept this syntax.

If it's dispreferred, though, I won't mind changing it.

>      (when parsebib-hashid-fields
> -      (push (cons "=hashid=" (secure-hash 'sha256 
> (parsebib--get-hashid-string fields))) entry))
> +      (push (cons "=hashid=" (secure-hash 'sha256 
> (parsebib--get-hashid-string fields))) entry)) ;why are you using strings for 
> keys?
>      entry))

I'm using string for keys because that's what bibtex.el also uses. It makes
it easier to use functions from bibtex.el on the data, if needs be.
(parsebib.el itself doesn't do this, but Ebib, my bib manager which uses
parsebib.el to read .bib files does).
  
> +           (new-fields (delq nil (mapcar (lambda (field)      ;mapcan can 
> avoid unnecessary consing, if that is important

The `mapcan` entry in the manual says something about the elements having
to be lists themselves, so if I understand that correctly, I'll have to
test to make sure it works.

> -  (pcase parsebib-TeX-cleanup-target
> +  (pcase parsebib-TeX-cleanup-target ;perhaps `pcase-exhaustive'?

Good point. I'll need to consider what I want to happen when there is no
match.

>      ('display (propertize str 'face 'italic))
>      ('markdown (concat "*" str "*"))
>      ('org (concat "/" str "/"))
> @@ -716,7 +716,7 @@ markup for bold text."
>      ('org (concat "*" str "*"))
>      ('plain str)))
>  
> -(defun parsebib--convert-tex-small-caps (str)
> +(defun parsebib--convert-tex-small-caps (str) ;why define this function at 
> all?

Another good point... It's parallel to `parsebib--convert-tex-bold` and
`parsebib--convert-tex-italics`, but you're right, there's no need for it.
Using `upcase` directly is clear enough. (And obviously, even if it
weren't, I should just used an alias...)

>        (nreverse res))))
> @@ -1227,11 +1227,10 @@ field, a `parsebib-error' is raised."
>  ENTRY is a CSL-JSON entry in the form of an alist.  ENTRY is
>  modified in place.  Return value is ENTRY.  If YEAR-ONLY is
>  non-nil, date fields are shortened to just the year."
> -  (mapc (lambda (field)
> -          (unless (stringp (alist-get field entry))
> -            (setf (alist-get field entry)
> -                  (parsebib-stringify-json-field (assq field entry) 
> year-only))))
> -        (mapcar #'car entry))
> +  (dolist (field entry)
> +    (unless (stringp (alist-get (car field) entry))
> +      (setf (alist-get (car field) entry)
> +            (parsebib-stringify-json-field (assq (car field) entry) 
> year-only))))
>    entry)

Is there any reason to prefer `dolist` over `mapc`? (I've never really
understood why both exist, TBH. I used to have an irrational preference for
`mapc`, but I've gotten over that. 😉)

> The question then is if we want to wait until the package is added to
> the core and track that or add it to ELPA right now?

I don't have an opinion on that, not being sure what the pros and cons of
each option would be.


-- 
Joost Kremers
Life has its moments