alphapapa / org-super-agenda

Supercharge your Org daily/weekly agenda by grouping items
GNU General Public License v3.0
1.33k stars 107 forks source link

Invalid org-super-agenda-groups-selector when using :take #216

Closed cashpw closed 9 months ago

cashpw commented 2 years ago

Relevant config details:

    (use-package! org-super-agenda
      ;; ...
      :hook
      ((org-agenda-mode . org-super-agenda-mode))
      :config
      (setq
       org-super-agenda-header-map evil-org-agenda-mode-map
       org-agenda-custom-commands '(("r" "Roam"
                                     ((alltodo "" ((org-agenda-overriding-header "")
                                                   (org-agenda-files
                                                    ;; ...
                                                    )
                                                   (org-super-agenda-groups
                                                    '((:name "Concepts"
                                                       :tag "concept")
                                                      (:take (9 (:name "References"
                                                                 :tag "reference")))
                                                      (:name "Quotes"
                                                       :tag "quote")
                                                      (:name "People"
                                                       :tag "person"))))))))))

Note that I don’t see any errors when I replace :take with :order-multi.

Full error message:

Invalid org-super-agenda-groups selector: (:name References :tag reference)
cashpw commented 2 years ago

Were there ever tests added for :take? I don't see anything in https://github.com/alphapapa/org-super-agenda/blob/master/test/test.el.

cashpw commented 2 years ago

I was able to fix this locally by changing org-super-agenda--group-dispatch-take to:

(cl-defun org-super-agenda--group-dispatch-take (items n-and-group)
  "Take N ITEMS that match selectors in GROUP.
If N is positive, take the first N items, otherwise take the last N items.
Note: the ordering of entries is not guaranteed to be preserved, so this may
not always show the expected results."
  (-let* ((n (car n-and-group))
          (group (cdr n-and-group))
          ((name non-matching matching) (org-super-agenda--group-dispatch items group))
          (take-fn (if (cl-minusp n) #'-take-last #'-take))
          (placement (if (cl-minusp n) "Last" "First"))
          (name (format "%s %d %s" placement (abs n) name)))
    (list name non-matching (funcall take-fn (abs n) matching))))
cashpw commented 2 years ago

I think it's the &rest that's causing it. The group variable appears to be wrapped by an extra set of parenthesis in my debug window (eg: ((:tag "concept")) rather than (:tag "concept")). I suspect &rest is adding this to make group into a list.

This is working for me:

(cl-defun org-super-agenda--group-dispatch-take (items (n group))
  "Take N ITEMS that match selectors in GROUP.
If N is positive, take the first N items, otherwise take the last N items.
Note: the ordering of entries is not guaranteed to be preserved, so this may
not always show the expected results."
  (-let* (((name non-matching matching) (org-super-agenda--group-dispatch items group))
          (take-fn (if (cl-minusp n) #'-take-last #'-take))
          (placement (if (cl-minusp n) "Last" "First"))
          (name (format "%s %d %s" placement (abs n) name)))
    (list name non-matching (funcall take-fn (abs n) matching))))
natejlong commented 9 months ago

Just to update this:

I ran into the same issue on the latest commit (https://github.com/alphapapa/org-super-agenda/commit/f4f528985397c833c870967884b013cf91a1da4a) using :take, and removing &rest as mentioned above still fixes it for me too.

alphapapa commented 9 months ago

Apparently I overlooked this issue until now. For future reference, feel free to ping me on any issues that I miss.

Thanks to all for the debugging. I'll merge the fix soon.

natejlong commented 9 months ago

No worries, more than understandable given the other 100 repos you manage haha. Thanks!

On Wed, Sep 20, 2023, at 15:58, Adam Porter wrote:

Apparently I overlooked this issue until now. For future reference, feel free to ping me on any issues that I miss.

Thanks to all for the debugging. I'll merge the fix soon.

— Reply to this email directly, view it on GitHub https://github.com/alphapapa/org-super-agenda/issues/216#issuecomment-1728529404, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYT37D5BFUM7KITGCCKB5TX3NYJVANCNFSM5N6GMI3Q. You are receiving this because you commented.Message ID: @.***>

alphapapa commented 9 months ago

Finally merged. Thanks to all.