ag91 / moldable-emacs

Adapting Emacs for moldable development
GNU General Public License v3.0
105 stars 8 forks source link

Issues with the "ElispListToOrgTable" mold & `me-alist-to-plist` #9

Closed kat-co closed 1 year ago

kat-co commented 2 years ago

I was working my way through the tutorial, and I get to the step where you are supposed to turn the list of buffers into a bar-chart. This fails for me, so I tried running ElispListToOrgTable with the following data:

((:buffer "Playground" :size 201)
 (:buffer " *Minibuf-1*" :size 0)
 (:buffer "Show Tutorials" :size 3510)
 (:buffer "*scratch*" :size 187)
 (:buffer "moldable-emacs.el" :size 47852)
 (:buffer " *Minibuf-0*" :size 0)
 (:buffer "*Messages*" :size 34483)
 (:buffer " *code-conversion-work*" :size 11)
 (:buffer " *Echo Area 0*" :size 0)
 (:buffer " *Echo Area 1*" :size 0)
 (:buffer "*Compile-Log*" :size 0)
 (:buffer "*Warnings*" :size 328)
 (:buffer "*direnv*" :size 0)
 (:buffer "core.el" :size 42803)
 (:buffer "contrib.el" :size 16916)
 (:buffer " *org-src-fontification:emacs-lisp-mode*" :size 90)
 (:buffer " *helm candidates:Emacs Commands history*" :size 7)
 (:buffer " *helm candidates:Emacs Commands*" :size 192024)
 (:buffer "*helm M-x*" :size 2276)
 (:buffer "*Completions*" :size 305)
 (:buffer "EvalSexp" :size 0))

I get this stack-trace:

Debugger entered--Lisp error: (wrong-type-argument sequencep :buffer)
  #f(compiled-function (it) #<bytecode 0x10c26e1>)(:buffer)
  mapcar(#f(compiled-function (it) #<bytecode 0x10c26e1>) (:buffer "Playground" :size 201))
  me-alist-to-plist(((:buffer "Playground" :size 201) (:buffer " *Minibuf-1*" :size 0) (:buffer "Show Tutorials" :size 3510) (:buffer "*scratch*" :size 187) (:buffer "moldable-emacs.el" :size 47852) (:buffer " *Minibuf-0*" :size 0) (:buffer "*Messages*" :size 34483) (:buffer " *code-conversion-work*" :size 11) (:buffer " *Echo Area 0*" :size 0) (:buffer " *Echo Area 1*" :size 0) (:buffer "*Compile-Log*" :size 0) (:buffer "*Warnings*" :size 328) (:buffer "*direnv*" :size 0) (:buffer "core.el" :size 42803) (:buffer "contrib.el" :size 16916) (:buffer " *org-src-fontification:emacs-lisp-mode*" :size 90) (:buffer " *helm candidates:Emacs Commands history*" :size 7) (:buffer " *helm candidates:Emacs Commands*" :size 192024) (:buffer "*helm M-x*" :size 2276) (:buffer "*Completions*" :size 305) (:buffer "EvalSexp" :size 0)))
  (if (ignore-errors (length (car l))) (me-alist-to-plist l) (me-alist-to-plist (-map #'-cons-to-list l)))
  (let* ((list (if (ignore-errors (length (car l))) (me-alist-to-plist l) (me-alist-to-plist (-map #'-cons-to-list l))))) (with-current-buffer buffername (org-mode) (erase-buffer) (me-insert-flat-org-table list) (setq-local self list)))
  (progn (get-buffer-create buffername) (let* ((list (if (ignore-errors (length (car l))) (me-alist-to-plist l) (me-alist-to-plist (-map #'-cons-to-list l))))) (with-current-buffer buffername (org-mode) (erase-buffer) (me-insert-flat-org-table list) (setq-local self list))) (ignore-errors (switch-to-buffer-other-window (get-buffer buffername))))
  (cond ((equal '(:then) '(:given)) (ignore-errors (and (>= (length l) 2) (listp (car l)) (or (consp (car l)) (--all\? (= (length it) (length ...)) l)) (or (-all\? #'stringp (list (caar l) (cdar l))) (-all\? #'stringp (car l)) (and (stringp (caar l)) (stringp (cdar l))) (--all\? (equal (-filter ... ...) (-filter ... it)) l))))) ((equal '(:then) '(:then)) (progn (get-buffer-create buffername) (let* ((list (if (ignore-errors ...) (me-alist-to-plist l) (me-alist-to-plist ...)))) (with-current-buffer buffername (org-mode) (erase-buffer) (me-insert-flat-org-table list) (setq-local self list))) (ignore-errors (switch-to-buffer-other-window (get-buffer buffername))))) (t :then))
  (pcase '(:then) ('(:given) (ignore-errors (and (>= (length l) 2) (listp (car l)) (or (consp (car l)) (--all\? (= (length it) (length ...)) l)) (or (-all\? #'stringp (list (caar l) (cdar l))) (-all\? #'stringp (car l)) (and (stringp (caar l)) (stringp (cdar l))) (--all\? (equal (-filter ... ...) (-filter ... it)) l))))) ('(:then) (progn (get-buffer-create buffername) (let* ((list (if (ignore-errors ...) (me-alist-to-plist l) (me-alist-to-plist ...)))) (with-current-buffer buffername (org-mode) (erase-buffer) (me-insert-flat-org-table list) (setq-local self list))) (ignore-errors (switch-to-buffer-other-window (get-buffer buffername))))) (_ :then))
  (let* ((l (list-at-point))) (pcase '(:then) ('(:given) (ignore-errors (and (>= (length l) 2) (listp (car l)) (or (consp (car l)) (--all\? (= ... ...) l)) (or (-all\? #'stringp (list ... ...)) (-all\? #'stringp (car l)) (and (stringp ...) (stringp ...)) (--all\? (equal ... ...) l))))) ('(:then) (progn (get-buffer-create buffername) (let* ((list (if ... ... ...))) (with-current-buffer buffername (org-mode) (erase-buffer) (me-insert-flat-org-table list) (setq-local self list))) (ignore-errors (switch-to-buffer-other-window (get-buffer buffername))))) (_ :then)))
  (let ((buffername (or nil "ElispListToOrgTable"))) (let* ((l (list-at-point))) (pcase '(:then) ('(:given) (ignore-errors (and (>= (length l) 2) (listp (car l)) (or (consp ...) (--all\? ... l)) (or (-all\? ... ...) (-all\? ... ...) (and ... ...) (--all\? ... l))))) ('(:then) (progn (get-buffer-create buffername) (let* ((list ...)) (with-current-buffer buffername (org-mode) (erase-buffer) (me-insert-flat-org-table list) (setq-local self list))) (ignore-errors (switch-to-buffer-other-window (get-buffer buffername))))) (_ :then))))
  (progn (let ((buffername (or nil "ElispListToOrgTable"))) (let* ((l (list-at-point))) (pcase '(:then) ('(:given) (ignore-errors (and (>= ... 2) (listp ...) (or ... ...) (or ... ... ... ...)))) ('(:then) (progn (get-buffer-create buffername) (let* (...) (with-current-buffer buffername ... ... ... ...)) (ignore-errors (switch-to-buffer-other-window ...)))) (_ :then)))))
  eval((progn (let ((buffername (or nil "ElispListToOrgTable"))) (let* ((l (list-at-point))) (pcase '(:then) ('(:given) (ignore-errors (and ... ... ... ...))) ('(:then) (progn (get-buffer-create buffername) (let* ... ...) (ignore-errors ...))) (_ :then))))) t)
  me-mold-run-then((:key "ElispListToOrgTable" :let ((l (list-at-point))) :given (:fn (ignore-errors (and (>= (length l) 2) (listp (car l)) (or (consp (car l)) (--all\? (= ... ...) l)) (or (-all\? #'stringp (list ... ...)) (-all\? #'stringp (car l)) (and (stringp ...) (stringp ...)) (--all\? (equal ... ...) l))))) :then (:fn (let* ((list (if (ignore-errors ...) (me-alist-to-plist l) (me-alist-to-plist ...)))) (with-current-buffer buffername (org-mode) (erase-buffer) (me-insert-flat-org-table list) (setq-local self list)))) :docs "You can produce an Org Table of the plist, list or..." :examples ((:name "Alist to Org table" :given (:type file :name "/tmp/my.el" :mode emacs-lisp-mode :contents "((\"Index\" \"Value\")\n (1 3)\n (2 9)\n (3  27))") :then (:type buffer :name "Org Table for list starting for (:Index 1 :Value 3..." :mode org-mode :contents "| Index | Value |\n|-------+-------|\n|     1 |     ...")) (:name "Cons list to Org table" :given (:type file :name "/tmp/my.el" :mode emacs-lisp-mode :contents "((\"Index\" . \"Value\")\n (1 . 3)\n (2 . 9)\n (3 . 27))") :then (:type buffer :name "Org Table for list starting for (:Index 1 :Value 3..." :mode org-mode :contents "| Index | Value |\n|-------+-------|\n|     1 |     ...")) (:name "Property list to Org table" :given (:type file :name "/tmp/my.el" :mode emacs-lisp-mode :contents "((:index 1 :value 3)\n (:index 2 :value 9)\n (:index...") :then (:type buffer :name "Org Table for list starting for (:index 1 :value 3..." :mode org-mode :contents "| index | value |\n|-------+-------|\n|     1 |     ..."))) :origin "/gnu/store/amsqdqzp1s0nmy185l9py4vd9hvqbihi-emacs-..."))
  me-mold()
  funcall-interactively(me-mold)
  call-interactively(me-mold record nil)
  command-execute(me-mold record)

I think this is an issue somewhere in this function:

https://github.com/ag91/moldable-emacs/blob/da95af66005664d88f159931837776694ab479e2/moldable-emacs.el#L127-L132

But I also think there is probably other issues with this mold:

(assoc :buffer
       '((:buffer "Playground" :size 201)
         (:buffer " *Minibuf-1*" :size 0)
         (:buffer "Show Tutorials" :size 3510)
         (:buffer "*scratch*" :size 187)
         (:buffer "moldable-emacs.el" :size 47852)
         (:buffer " *Minibuf-0*" :size 0)
         (:buffer "*Messages*" :size 34483)
         (:buffer " *code-conversion-work*" :size 11)
         (:buffer " *Echo Area 0*" :size 0)
         (:buffer " *Echo Area 1*" :size 0)
         (:buffer "*Compile-Log*" :size 0)
         (:buffer "*Warnings*" :size 328)
         (:buffer "*direnv*" :size 0)
         (:buffer "core.el" :size 42803)
         (:buffer "contrib.el" :size 16916)
         (:buffer " *org-src-fontification:emacs-lisp-mode*" :size 90)
         (:buffer " *helm candidates:Emacs Commands history*" :size 7)
         (:buffer " *helm candidates:Emacs Commands*" :size 192024)
         (:buffer "*helm M-x*" :size 2276)
         (:buffer "*Completions*" :size 305)
         (:buffer "EvalSexp" :size 0)))

=> (:buffer "Playground" :size 201).

Traditionally, Lisps have consed additional values onto an alist because assoc style functions will stop once they find the first match. This is how alists are updated.

Therefore, I think it may be incorrect and misleading to call a function named me-alist-to-plist since this is not really an alist despite being in an alist shape, and this may be why there's an error.

I haven't continued digging into what shape of list me-insert-flat-org-table expects.

ag91 commented 2 years ago

Uhm, it is weird I cannot reproduce this. Definitely those functions are fragile and badly named. The idea behind them is that we want plists or alists that have all consistent keys. This because from ((:a a :b b)) I can easily identify the headers of the table to be "a" and "b". What happens if you run:

(--> '((:buffer "Playground" :size 201)
       (:buffer " *Minibuf-1*" :size 0)
       (:buffer "Show Tutorials" :size 3510)
       (:buffer "*scratch*" :size 187)
       (:buffer "moldable-emacs.el" :size 47852)
       (:buffer " *Minibuf-0*" :size 0)
       (:buffer "*Messages*" :size 34483)
       (:buffer " *code-conversion-work*" :size 11)
       (:buffer " *Echo Area 0*" :size 0)
       (:buffer " *Echo Area 1*" :size 0)
       (:buffer "*Compile-Log*" :size 0)
       (:buffer "*Warnings*" :size 328)
       (:buffer "*direnv*" :size 0)
       (:buffer "core.el" :size 42803)
       (:buffer "contrib.el" :size 16916)
       (:buffer " *org-src-fontification:emacs-lisp-mode*" :size 90)
       (:buffer " *helm candidates:Emacs Commands history*" :size 7)
       (:buffer " *helm candidates:Emacs Commands*" :size 192024)
       (:buffer "*helm M-x*" :size 2276)
       (:buffer "*Completions*" :size 305)
       (:buffer "EvalSexp" :size 0))
     me-alist-to-plist
     )

? It is suspicious your mapcar(#f(compiled-function (it) #<bytecode 0x10c26e1>) (:buffer "Playground" :size 201)) because it seems it is going in the else branch here https://github.com/ag91/moldable-emacs/blob/f3fe8e3/molds/core.el#L179, which should not happen.

kat-co commented 2 years ago

That gives an error for me too.

This is as close to a 100% assertion I can make that the issue is not on my end:

katco says: guix environment -C --pure -ETERM --no-cwd --ad-hoc emacs emacs-moldable-emacs -- emacs --batch --eval "(require 'moldable-emacs)" --eval "(me-alist-to-plist '((:a "A")))"
Loading /gnu/store/amsqdqzp1s0nmy185l9py4vd9hvqbihi-emacs-moldable-emacs-0.0.0-1.822965d/share/emacs/site-lisp/moldable-emacs-0.0.0-1.822965d/moldable-emacs-autoloads...
Loading /gnu/store/g3m09phwjc4anjq4j8gz14chsk5gwqjz-emacs-dash-2.19.1/share/emacs/site-lisp/dash-2.19.1/dash-autoloads...
Loading /gnu/store/dz7495gb19jhxk10yvcf3dxzhvkgxsrz-emacs-s-1.12.0/share/emacs/site-lisp/s-1.12.0/s-autoloads...
Loading /gnu/store/dvdrlq6f9gs43axk7js1c337qmm51ih3-emacs-async-1.9.4/share/emacs/site-lisp/async-1.9.4/async-autoloads...
Wrong type argument: sequencep, :a

What this is doing is starting a container with

and evaling (me-alist-to-plist '((:a "A"))).

ag91 commented 2 years ago

Mmm, can I run that with the scm file of the PR as well?

Anyway, quick attempt: can you try to rerun the test redefining me-alist-to-plist like this

(defun me-alist-to-plist (alist)
  "Convert ALIST to a `plist'."
  (let ((keys (ignore-errors (--map (intern (concat ":" it)) (car alist)))))
    (if (and keys (ignore-errors (= (length (car alist)) (length (-filter #'stringp (car alist))))))
        (--map (-flatten (-zip-lists keys it)) (cdr alist))
      alist)))

I suspect that the if-let* has a different semantic than on my system. I actually don't know where I get that macro from, so I tried to remove it (this is just an ugly implementation to see if we can patch up your error)

kat-co commented 2 years ago

Mmm, can I run that with the scm file of the PR as well?

Yes, if you have Guix installed, you can run guix environment -C --pure -ETERM --no-cwd --ad-hoc emacs --load=./guix.scm -- emacs --batch --eval "(require 'moldable-emacs)" --eval "(me-alist-to-plist '((:a "A")))"

With the function redefined as is in your comment, it works!

For clarity, I would still ask you to reconsider the terminology in some of the variables, comments, and function names. Not only because of the inputs, but because of the outputs. In this case, what's returned from me-alist-to-plist appears not not to be a plist, but a list of plists.

I can't give any more guidance on that until I get a little more familiar with things.

kat-co commented 2 years ago

It looks like it's a macro defined in subr-x. So it would just require a (require 'subr-x) I think.

ag91 commented 2 years ago

For clarity, I would still ask you to reconsider the terminology in some of the variables, comments, and function names. Not only because of the inputs, but because of the outputs. In this case, what's returned from me-alist-to-plist appears not not to be a plist, but a list of plists.

I can't give any more guidance on that until I get a little more familiar with things.

Guidance appreciated. This also requires I book some time for refactoring things. I will need to revisit those functions anyway, because detecting/converting Org Tables seems too expensive with the default functions available in Emacs.

ag91 commented 1 year ago

It seems I have addressed these points in previous changes