Hello, Dan Drake <dan.dr...@gmail.com> writes:
> I asked about a way to specify a time when using org-resolve-clock instead > of a number of minutes: > > https://lists.gnu.org/archive/html/emacs-orgmode/2020-01/msg00010.html > > I've implemented this myself and a patch is attached. Comments welcome -- > my change works, but I'm not sure about coding style, and right now there's > no error checking. Thank you. Some comments follow. > I marked the patch as a tiny change, but it does add a new menu option and > behavior to org-resolve-clock, so there may be an argument that it's not, > from a user perspective, a "tiny change", but code-wise it's quite simple: > the core logic really isn't more than "ask the user for a time and > subtract". TINYCHANGE is only about the number of non-trivial lines of code in your patch (15 or so). > +(defun time-to-mins-to-keep (start-time) > + "Asks the user for a time and returns the number of minutes > +from START-TIME to that time." > + (floor (/ (float-time > + (time-subtract (org-read-date t t) start-time)) 60))) The name of the function is wrong, it should start with "org-clock". Also, docstring's first line must contain a full sentence. Anyway, this is used only once in your patch, I suggest to inline in instead. > + (or (and (memq ch '(?k ?K)) > + (read-number "Keep how many minutes? " default)) > + (and (memq ch '(?t ?T)) > + (time-to-mins-to-keep last-valid)))) See above about inlining. Would you mind adding an ORG-NEWS entry, and, if possible, writing a test? Regards, -- Nicolas Goaziou
