Re: Month-week and quarter-week datetrees (RFC and package announcement)

[Ihor Radchenko <yantar92@posteo.net>]

Jack Kamm <jackkamm@gmail.com> writes:

> I attach a pair of patches for this.

Thanks! See my comments inline.

> +(defun org-datetree-find-create-entry
> +    (time-grouping d &optional keep-restriction)
> +  "Find or create an entry for date D.
> +TIME-GROUPING specifies the grouping levels of the datetree, and
> +should be a subset of `(year quarter month week day)'.  Weeks are
> +assigned to years according to ISO-8601.  If TIME-GROUPING
> +contains both `month' and `week', then weeks are assigned to the
> +month containing Thursday, for consistency with the ISO-8601
> +year-week rule.  If TIME-GROUPING contains `quarter' and `week'
> +but not `month', quarters are defined as 13-week periods;
> +otherwise they are defined as 3-month periods.
>  
> -If MATCH-TITLE is non-nil, REGEX-TEMPLATE is matched against
> -heading title and the exact regexp matched against heading line is:
> +If KEEP-RESTRICTION is non-nil, do not widen the buffer.  When it
> +is nil, the buffer will be widened to make sure an existing date
> +tree can be found.  If it is the symbol `subtree-at-point', then
> +the tree will be built under the headline at point."

Please also document how `org-datetree-add-timestamp' affects this function.

> +     ;; Support the old way of tree placement, using a property
> +     (cond
> +      ((seq-set-equal-p time-grouping '(year month day))
> +       "DATE_TREE")
> +      ((seq-set-equal-p time-grouping '(year month))
> +       "DATE_TREE")
> +      ((seq-set-equal-p time-grouping '(year week day))
> +       "WEEK_TREE")))

It would be a good idea to add a few tests for this scenario.
To make sure that refactoring did not break things.

> +        ;; Support the old way of tree placement, using a property
> +        (let ((prop (and legacy-prop (org-find-property legacy-prop))))
> +          (if prop
> +              (progn
> +                (goto-char prop)
> +	        (org-narrow-to-subtree)
> +                (setq tree (car (org-element-contents (org-element-parse-buffer 'headline)))))
> +            (setq tree (org-element-parse-buffer)))))

Why do you need object granularity by default (second call to
`org-element-parse-buffer')?
Also, more importantly, do you have to run the full parsing here? Maybe
utilize `org-element-cache-map' instead? Full parsing is going to be
much slower.

> +      (cl-loop
> +       for pair in hier-pairs
> +       do
> +       (setq tree
> +             (org-datetree--find-create-subheading
> +              (cadr pair) (car pair) tree)))

It is undocumented in the `org-datetree--find-create-subheading'
docstring that it returns something.

-- 
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>