Closed jun0 closed 9 months ago
Sorry, last-minute change: it's better to require org-gtd instead of individual modules like org-gtd-archive. See commit log for details.
That's interesting, I hadn't considered doing it that way, and I was going mad trying to put the right requires in all the right places.
Do you have any idea if/how we could write tests or otherwise ensure this works in a CI pipeline or during development, in an automated fashion?
That's interesting, I hadn't considered doing it that way, and I was going mad trying to put the right requires in all the right places.
Yeah, it's a bit tricky, but I think this really is the right place for this require
. The reason with-org-gtd-context
makes sense at all, despite the apparent cyclic dependency, is that the dependency is dynamic: the macro is only used inside defuns and not needed at load time. So it's only natural that the require
is also dynamic.
Do you have any idea if/how we could write tests or otherwise ensure this works in a CI pipeline or during development, in an automated fashion?
We could do something like the "Steps to Reproduce" I wrote above: start a fresh Emacs instance with minimal config and invoke one of the autoloaded functions, restart Emacs and repeat. Perhaps write a test/autoload-test.el
that repeatedly starts new Emacs instances as subprocesses, let the subprocess-Emacs write out test results to a file or stdout, and parse that in test/autoload-test.el
?
I've never worked with Eldev, buttercup, or GitHub CI before, but if you can give me some quick pointers (how/when tests are triggered in your Eldev workflow or the CI framework; how it finds test files; and where I could put files meant only for the subprocess-Emacs) maybe I can put something together. Be warned I'm a bit busy over the next week or so, though.
So... it took a while to find the time to iron out the details, but this PR now includes automated testing for the autoload functionality. What do you think?
I skipped tests for several commands that need some setup (and I'm not knowledgeable enough to set them up myself), I hope that's fine:
org-gtd-delegate-agenda-item
org-gtd-delegate-item-at-point
org-gtd-review-area-of-focus
org-gtd-project-cancel
Oh wow, thank you! I was preparing to spend most of tomorrow reviewing this, but this new PR is fantastic.
And it's fine that you're leaving out tests for these commands; only review-area-of-focus
has a reasonable chance of triggering an autoload situation, but I think this PR should cover that case anyway; I'll add that test in the future.
Thank you again for this, it's going to help a lot of people!
This PR fixes the problem where you get
Symbol’s value as variable is void: org-gtd-archive-location
or similar such-and-such-is-undefined errors if you don't do an explicit(require 'org-gtd)
, making autoloads useless.Steps to reproduce
Short version: do
M-x org-gtd-engage
when org-gtd is installed and activated but not loaded.Long version: assuming Emacs 29.1, make a fresh directory containing an
init.el
file with the following contents and nothing else.emacs --init-directory=<that directory>
, wait for it to download and install org-gtd.M-x org-gtd-engage
.Result:
org-gtd-engage: Symbol’s value as variable is void: org-gtd-archive-location
Cause
package-activate-all
at startup, which loadsorg-gtd-autoloads.el
, which setsorg-gtd-engage
to be loaded fromorg-gtd-agenda.el
.org-gtd-engage
useswith-org-gtd-context
, butorg-gtd-agenda.el
doesn't load all the packages this macro requires, in particularorg-gtd-archive.el
.Similar problems abound for other autoloaded functions. The root of the problem is that
with-org-gtd-context
tightly couplesorg-gtd-agenda.el
,org-gtd-projects.el
,org-gtd-delegate
, andorg-gtd-core.el
, but these modules are not (cannot be) set up to require each other for fear of recursiverequire
.Fix
I fixed it by adding
require
fororg-gtd-agenda
et al. from the code generated bywith-org-gtd-context
.I also added a missing
(require 'f)
inorg-gtd-core.el
while I was at it. You can tickle this bug by removingorg-gtd-core.elc
before doingorg-gtd-engage
, which complains that-flatten
is undefined.