Earl Chase <[email protected]> writes:

Thanks for the update!
The next batch of comments below.

> (test-org-link/search-first-pass): New test for `org-link-search'.
> Assert `org-link-search' can find exact headline matches.n

.n typo

> (test-org-link/search-second-pass): New test for org-link-search.

`org-link-search'.

> +(cl-defun org-link-normalize-string (string &optional (ignored-contents 
> '(statistics-cookies search-syntax)))
> +  "Remove contigous white spaces and IGNORED-CONTENTS from STRING.
> +IGNORED-CONTENTS is a list of extra things to remove.  It can be any
> +subset of (statistics-cookies search-syntax pipe-chars):
> +- statistics-cookies are statistics cookie strings
> +- search-syntax is # in #heading and enclosing () in (ref)
> +- pipe-chars are | characters that may clash with table syntax"
> +  (let ((functions (list (cons 'statistics-cookies 
> #'org-link--remove-statistics-cookies)
> +                         (cons 'search-syntax 
> #'org-link--remove-search-syntax)
> +                         (cons 'ignore-pipes #'org-link--remove-pipes)
> +                         (cons 'remove-spaces 
> #'org-link--remove-contigous-spaces)
> +                         (cons 'trim #'org-trim)))

'ignore-pipes feels out of place. Maybe 'remove-pipes?
Or even 'pipes. You have 'statistics-cookies and 'search-syntax just
naming what is removed, but then 'ignore-pipes, 'remove-spaces, and
'trim - action to perform. This feels inconsistent.

> +(defun test-regular-org-link-heading-search-string (heading)
> +  "Helper function for `test-org-link-search'.
> +Inserts HEADING into an org
> +buffer and then returns the heading value
> +as a org-link search string."
> +  (org-test-with-temp-text heading
> +    (org-link-heading-search-string)))

We usually use test-org-link prefix there. test-regular-org-link does
not follow that convention.
Also, this is very short and used exactly once in a single test. You may
as well just use cl-labels of cl-macrolet instead.

> +(defun test-org-link-heading-search-string-remove-pipes (heading)
> +  "Helper function for `test-org-link-search'.
> +Inserts HEADING into an org
> +buffer and then returns the heading value
> +as a org-link search string.
> +Replaces pipe chars with spaces
> +in order to simulate the way
> +org-clock removes spaces from
> +headings when it creates links
> +for a clocktable."
> +  (org-test-with-temp-text heading
> +    (org-link-heading-search-string nil t)))

Same thing about single use.

> +(cl-defun test-org-link-search (search-string-creator)
> +  "Returns a closure that can be used to test `org-link-search'.
> +SEARCH-STRING-CREATOR should be a function
> +that returns an org-link search string."
> +  (lambda (buffer-text headline-to-find)
> +    (let ((org-link-search-must-match-exact-headline nil)
> +          (org-todo-regexp "TODO"))
> +      (org-test-with-temp-text buffer-text
> +        (org-link-search (funcall search-string-creator headline-to-find))
> +        (should (string-equal (buffer-substring-no-properties (point) 
> (line-end-position)) headline-to-find))))))

Could be a macro. That will simplify things.

> +(defalias 'test-org-link-search-basic (test-org-link-search 
> #'test-regular-org-link-heading-search-string))
> +
> +(defalias 'test-org-link-search-replace-pipe-chars (test-org-link-search 
> #'test-org-link-heading-search-string-remove-pipes))

Could port into cl-labels/cl-macrolet.

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

Reply via email to