Tor-björn Claesson <[email protected]> writes: >> The docstrings should indeed be improved. Writing the docstrings often >> reveals the edge cases missed in the code itself. > > Thanks, I have tried to improve the docstrings.
Thanks for the update! One simple thing you can do to improve the docstrings further is M-x checkdoc inside ol.el file. This will make Emacs check for typical docstring conventions. The goal should be (1) having 0 checkdoc warnings; (2) making sure that one does not need to read the function body to understand what the inputs are and what the returned value is. > ;;; Main macro definition > (cl-defmacro org-menu-define > (name arglist docstring description contents default-action &body body) > "Define an org menu NAME. > > A function called NAME will be created to activate the menu. There are also a bunch of variables created. We need to document those. > ARGLIST is the name of the arguments given to this function. *list of arguments > DOCSTRING is the menu docstring. > > DESCRIPTION is a short menu title, shown to explain the function > of the menu while in use. You should specify that it is a string. > BODY is optional and can be used to set up the interactive > environemnt and validate arguments." I think that we need to mention that BODY is evaluated regardless of the menu system used, even for default action when org-menu-mode is disabled. We should also tell that (interactive ...) is allowed in the body, but optional. > ;; Make sure we have an interactive body > ,@(if (and (listp body) > (and (listp (car body)) > (eq (caar body) 'interactive))) > body > `((interactive (list (org-element-context))) > ,@body)) Why do we force (org-element-context) as the default interactive specification? What if someone does (org-menu-define foo () "Dummy menu" "Dummy" [] (message "Am I working?")) Also, looking at the above example, I feel like two strings in a row appear confusing. Maybe we can have something more descriptive like (org-menu-define foo () "Dummy menu" :description "Dummy" :menu [] :default-action (message "Am I working?")) Description might probably be optional, using the first line from the docstring by default. > (transient-setup > (quote ,name) nil nil > :scope (list > ,@(mapcan (lambda (parameter) > (list (intern > (concat ":" > (symbol-name parameter))) > parameter)) > (org-menu--strip-argument-decorators > arglist)))) > > :setup-children > (lambda (_) > (transient-parse-suffixes > ',name > (org-menu--bind-specification > (mapcar > (lambda (arg) > `(,(intern (concat "!" (symbol-name arg))) > (plist-get (transient-scope) > ,(intern (concat ":" (symbol-name arg)))))) > `',@(org-menu--strip-argument-decorators ',arglist)) > ,menu-actions))) We are doing a double duty here. Why not storing !argument in transient scope from the very beginning? > I have gotten rid of complete-arglist, changed the org-cite-basic-follow > definition to specify the complete argument list, use current-prefix-arg > for the switch thing, and made a function to filter out &optional etc. > This feels better, is it good enough? Looks better, yes. > (defun org-menu--strip-argument-decorators (arglist) > "Return a copy of ARGLIST without &optional, &body, &key, &aux, or &rest." > (seq-filter > (lambda (elt) > (not (or (eq elt '&optional) > (eq elt '&body) > (eq elt '&rest) > (eq elt '&aux) > (eq elt '&key)))) > arglist)) I think we should not bother with &aux and &key - those can have funny forms like &key (foo "default" aux), which will require more complicated treatment. Let's not bother and stick to stock Elisp arglist format, omitting cl style. > (org-menu--defcustom-default-action > ,menu-default-action > ',default-action > ,name > ',(org-menu--strip-argument-decorators arglist)) We do not need to strip decorators here I think. The whole purpose of passing ARGLIST here is to make it appear in the docstring of the new defined defcustom. &optional/&rest will actually be useful there. >> I think that we should also put group names like "Open" or "Copy" as >> non-clickable (menu-item ITEM-NAME), maybe even adding a separator below. > > Good idea. I improved the function to produce a (flat) menu, with > separator for all but the first heading. Should this be flat, or should > we have submenues? This could maybe depend on how many entries there are. I am not sure yet. I want to try how the new menu system will work with more complex menus like export dispatch. But let's do it later, after cleaning up the existing code. -- Ihor Radchenko // yantar92, Org mode maintainer, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92>
