Re: Upstreaming parsebib.el

[Philip Kaludercic]

Joost Kremers <joostkremers@fastmail.fm> writes:

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

No problem, just wanted to suggest it if you didn't know about 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).

That's fine then.  Was there a comment that explained this somewhere?

>> +           (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.

The idea is to replace

  (delq nil (mapcar (lambda (x) (if (fn x) x nil))  list))

with

  (mapcan (lambda (x) (if (fn x) (list x) nil)) list)

as you are providing the cons-cells that the resulting list will be made
up of (instead of creating a cons-cell for each element of the old list
and then discarding some of those).

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

With `pcase-exhaustive' you would raise an error, but my understanding
was that this should be interpreted as an invalid state anyway.  So it
is better to make it explicit and fail early than continuing on with a
nil value.

>>      ('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...)

1+

>>        (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. 😉)

I think that from a readability perspective, it is useful to mention
what you are looping over at the beginning, but the other point is that
it is (slightly) more expensive to funcall a closure than to use
`while'.  But again, this is just a suggestion, any strong opinion from
your side overrides 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.

It wouldn't make much of a difference in the end.  The biggest thing
that I can think of is that it might be slightly annoying to move from
an externally tracked package to a file tracked in emacs.git, but that
has been done before.

A different perspective is to ask if there is a hurry to merge this.  As
Emacs 31 is  not about to be cut soon, I would say that it should be
fine to wait a bit after the matter has been discussed with whoever is
responsible for merging parsebib into emacs.git.  Do you know who that
is, btw?