Hi Ihor and Karthik,
> Thanks! > Applied, onto main. > LRA, thanks for the fix. Glad to be of assistance! > P.S. We really need tests for org-persist. Yeah I did note a couple of pretty straight-forward opportunities for clean-up wrt. redundancies and readability, but without regression-tests and lacking confidence that I understood everything I thought it was best to keep the patch as simple as possible. I would've felt pretty stupid if I accidentally introduced a new regression in my regression-fix haha. I'll include some of the stuff from my notes below in case it's of interest to anybody working on org-persist. I hope it isn't just noise and that it can be of use :) Regards, LRA ----- (These are all relative to the org-latex-preview dev-branch and not main, but I think all the code in question applies to both, just the linenums might be off.) 1. I noticed when testing on upstream that there are a ton of redundant calls to `o-p--normalize-container' throughout the package. I saw that you've already dealt with the redundancy in your branch Karthik, so maybe it's unimportant but I think the redundant calls also just hurt readability somewhat. A couple cases of #+begin_example elisp (defun somefun (container ... ) (let ((container (org-persist--normalize-container container)) ... ))) ;; And then every call will have ... (somefun (org-persist--normalize-container container)) ... #+end_example 2. The bits handling the shape of container in `o-p--add-to-index' and `o-p--remove-from-index' are both redundant, as all calls to these functions use containers read from the index or from `o-p--get-collection', in both cases there's already logic ensuring it's a list of lists. This change worked fine in my limited testing: Replacing =lisp/org-persist.el:L576-9= and =L592-5= #+begin_example elisp (dolist (cont (cons container container)) #+end_example 3. Maybe I'm just an idiot, but the use (or maybe just the name) of `o-p--get-collection' was a bit confusing to me, as it's used both to find and /create/ indices. E.g. this bit of logic in `o-p-register' was particularly confusing where it's used in a let /before/ a conditional that determines whether the assignment is actually used for anything. =lisp/org-persist.el:L1006-8= #+begin_example elisp (let ((collection (org-persist--get-collection container associated misc))) (when (and expiry (not inherit)) (when expiry (plist-put collection :expiry expiry)))) #+end_example With the redundant inner `when', it reads a bit like the let was supposed to be between the two, which I think makes sense if the first condition is an `or' instead of `and'? Although I'm not sure I actually figured out how this function or the inherit keyword works. 4. > Ihor, `org-persist--find-index' is singularly confusing. I've been over > it with edebug a few times and still can't figure out what it's doing, > because `container' means different things at different points in the > function because of the macro `org-persist-collection-let'. Could it be > simplified in some way? I had the exact same confusion motivating my attempt at rewording the comment. On my machine and in my limited testing, dropping the second use of the macro completely worked fine: Replacing =lisp/org-persist.el:L552-4= #+begin_example elisp (lambda (cont) (member cont (plist-get r :container))) #+end_example 5. Another way to simplify `o-p--find-index' (and I think also get pretty close to achieving full consistency on the container shape), would be to put this bit from the start of `o-p--get-collection' at the start of `-read', `-write-all' and `-unregister' (or maybe in a (if (not inner) .. ) at the end of `o-p--normalize-container'?): #+begin_example elisp (unless (and (listp container) (listp (car container))) (setq container (list container))) #+end_example This would cover all calls to`o-p--find-index' and allow you to revert the changes in both my patch and 7fd80991c3 I think. This is sort-of what I was thinking of wrt. rewriting `o-p--normalize-container' in my original e-mail. -----
