refactor: expand docstrings, simplify internals, drop ht and dash#18
Merged
Conversation
Rewrite the package with publication-quality docstrings and remove external dependencies in favour of Emacs built-ins. - Replace `ht` and `dash` with built-in hash tables, `seq`, and `cl-lib`. The package now requires only Emacs 28.1+. - Expand every public docstring with usage details, side effects, and examples; clarify the internal ones. - Simplify several internals: rewrite `--select-non-side-window` with `cl-find-if-not`; rename `--list-starts-with` to `--prefix-p` and reimplement with `seq-take`; drop the redundant "space-tree" sentinel key from the tree's root; inline `--get-parent-ht-at`; factor `go-right`/`go-left` through a shared `--go-by` helper; factor digit-address parsing into `--parse-digit-address`; simplify `switch-space-by-name`'s alist juggling. - Fix `paste-workspace`'s `(interactive)` so the prefix arg actually reaches `inplace`; drop the broken `(interactive)` from `switch-or-create`; require a match in `switch-space-by-name` so it can't pass nil downstream. - Replace the 19 hand-written lambdas in the README example with the existing `space-tree-to-*`/`-sub-*`/`-sub-sub-*` convenience commands; add a short section documenting that family. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the Emacs Lisp space-tree package to remove third-party dependencies (ht, dash), expand user-facing documentation, and simplify several internal helpers while updating tests and documentation accordingly.
Changes:
- Dropped
ht/dashand replaced usage with built-in hash-table,seq, andcl-libutilities. - Added/expanded docstrings and refactored internals (history processing, digit-address parsing, modeline rendering, navigation helpers).
- Updated ERT tests, README examples, and Eask metadata to match the new internals and dependency set.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
space-tree.el |
Removes external deps; adds helpers (--prefix-p, --parse-digit-address, --go-by), refactors state/modeline/navigation, expands docstrings. |
test/space-tree-test.el |
Updates tests to use built-in hash-table APIs; adds coverage for --parse-digit-address and pre-init modeline rendering. |
README.md |
Updates example config to use new convenience commands; documents command families; updates dependency notes. |
Eask |
Removes ht/dash from declared dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+580
to
+583
| (let* ((siblings (space-tree--siblings-of space-tree-current-address)) | ||
| (i (seq-position siblings (car (last space-tree-current-address)))) | ||
| (j (mod (+ i delta) (length siblings)))) | ||
| (space-tree-switch-current-level (nth j siblings)))) |
Owner
Author
There was a problem hiding this comment.
Added space-tree--validate-address and now call it at the top of space-tree--go-by. Pre-init / inconsistent-state navigation now signals a clear user-error instead of a low-level type error. Fixed in f9af373.
Comment on lines
+86
to
+90
| | Command family | What it switches to | | ||
| |-----------------------------------------|----------------------------------------| | ||
| | `space-tree-to-1` … `space-tree-to-9` | Top-level space N | | ||
| | `space-tree-sub-1` … `space-tree-sub-5` | Sub-space N of the current top-level | | ||
| | `space-tree-sub-sub-1` … `-sub-sub-5` | Sub-sub-space N of the current sub | |
Owner
Author
There was a problem hiding this comment.
Spelled out the full space-tree-sub-sub-5 name in that row for consistency. Fixed in f9af373.
Comment on lines
629
to
+647
| (defmacro space-tree--def-level-commands (prefix doc-template count &rest address-body) | ||
| "Define COUNT interactive switch commands named PREFIX-1 through PREFIX-COUNT. | ||
| DOC-TEMPLATE is a format string taking one integer arg for the docstring. | ||
| ADDRESS-BODY are forms that compute the address list; the variable `n' | ||
| is bound to the space number." | ||
| "Define COUNT interactive switch commands sharing a common PREFIX. | ||
|
|
||
| For each integer i in 1..COUNT, define a function named `PREFIX-i' | ||
| that calls `space-tree-switch-or-create' with the address produced | ||
| by ADDRESS-BODY, with the variable `n' bound to i. | ||
|
|
||
| DOC-TEMPLATE is a `format' template taking one integer argument; it | ||
| is used as the docstring of each generated command." | ||
| (declare (indent 3)) | ||
| `(progn | ||
| ,@(mapcar | ||
| (lambda (i) | ||
| `(defun ,(intern (format "%s-%d" prefix i)) () | ||
| ,(format doc-template i) | ||
| (interactive) | ||
| (let ((n ,i)) | ||
| (space-tree-switch-or-create ,@address-body)))) | ||
| (number-sequence 1 count)))) | ||
| (lambda (i) | ||
| `(defun ,(intern (format "%s-%d" prefix i)) () | ||
| ,(format doc-template i) | ||
| (interactive) | ||
| (let ((n ,i)) | ||
| (space-tree-switch-or-create ,@address-body)))) | ||
| (number-sequence 1 count)))) |
Owner
Author
There was a problem hiding this comment.
All convenience commands funnel through space-tree-switch-or-create, which now calls space-tree--validate-address on the incoming address. A space-tree-sub-N invoked at depth 1 yields (nil N), which the validator rejects with a clear user-error. No need to instrument the macro itself. Fixed in f9af373.
Comment on lines
+331
to
+335
| "Snapshot the current window state into the current space." | ||
| (puthash space-tree-current-address | ||
| (window-state-get) | ||
| space-tree-address-wconf-tbl)) | ||
|
|
Owner
Author
There was a problem hiding this comment.
Made space-tree--save-wconf a no-op when space-tree-current-address is nil, so it can no longer create a nil-keyed entry. Fixed in f9af373.
Address Copilot review feedback on PR #18. - Add `space-tree--validate-address' which signals a clear user-error when an address is nil (space-tree not initialised) or contains a nil component (level-based convenience command invoked from too shallow a depth). - Call the validator at entry to `space-tree-switch-or-create' (which every navigation command funnels through) and in `space-tree--go-by'. - Make `space-tree--save-wconf' a no-op when no current space is selected, instead of writing a nil-keyed entry. - Spell out the full `space-tree-sub-sub-5' name in the README convenience-command table for consistency. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiply
added a commit
that referenced
this pull request
May 25, 2026
Address Copilot review feedback on PR #18. - Add `space-tree--validate-address' which signals a clear user-error when an address is nil (space-tree not initialised) or contains a nil component (level-based convenience command invoked from too shallow a depth). - Call the validator at entry to `space-tree-switch-or-create' (which every navigation command funnels through) and in `space-tree--go-by'. - Make `space-tree--save-wconf' a no-op when no current space is selected, instead of writing a nil-keyed entry. - Spell out the full `space-tree-sub-sub-5' name in the README convenience-command table for consistency.
chiply
added a commit
that referenced
this pull request
May 25, 2026
refactor: expand docstrings, simplify internals, drop ht and dash
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Publication-quality polish on space-tree. No external dependencies, expanded user-facing docstrings, and simpler internals.
htanddashdependencies. The package now requires only Emacs 28.1+; built-in hash tables,seq, andcl-libreplace the third-party helpers. One fewer transitive install for downstream users.init's destructive behaviour, what the internal "clipboard" actually is). Several misleading internal docstrings are also clarified.--select-non-side-windowusingcl-find-if-not; rename--list-starts-withto--prefix-pand reimplement withseq-take; drop the redundant"space-tree"sentinel key from the tree root; inline--get-parent-ht-at; factorgo-right/go-leftthrough a shared--go-by; factor digit-address parsing into--parse-digit-address; simplifyswitch-space-by-name's alist juggling.paste-workspacenow reads its prefix arg correctly ((interactive "P")), soC-u M-x space-tree-paste-workspaceactually pastes in place.switch-or-create's broken(interactive)form is removed (it required an arg the spec couldn't supply).switch-space-by-namenow requires a match so it can't silently pass nil toswitch-or-create.space-tree-to-N/space-tree-sub-N/space-tree-sub-sub-Nconvenience commands; add a short section documenting that family.Test plan
eask compilecleaneask lint package/checkdoc/elisp-lint/relintall exit 0eask test ert test/space-tree-test.el— 56 tests, all passing (test suite extended for--prefix-p,--parse-digit-address, and the pre-init empty-modeline case)space-tree-paste-workspacewithC-u🤖 Generated with Claude Code