imports
This commit is contained in:
440
tasks/12-imports-block-support-FIX.md
Normal file
440
tasks/12-imports-block-support-FIX.md
Normal file
@@ -0,0 +1,440 @@
|
||||
# Task 12: Imports Block Support - FIX
|
||||
|
||||
## Issue Found
|
||||
|
||||
During implementation of Task 12, tests failed because imports were being placed **inside** the value wrapper, creating invalid Elixir syntax.
|
||||
|
||||
### The Problem
|
||||
|
||||
When executing code with `:results value`, the code is wrapped like this:
|
||||
|
||||
```elixir
|
||||
result = (
|
||||
user_code_here
|
||||
)
|
||||
IO.puts(inspect(result, ...))
|
||||
```
|
||||
|
||||
The initial implementation prepended imports to `full-body` before wrapping, resulting in:
|
||||
|
||||
```elixir
|
||||
result = (
|
||||
import Enum # <- INVALID: import cannot be inside expression
|
||||
map([1,2,3], &(&1 * 2))
|
||||
)
|
||||
IO.puts(inspect(result, ...))
|
||||
```
|
||||
|
||||
This is invalid Elixir because `import`, `alias`, `use`, and `require` are not expressions and cannot appear inside parentheses.
|
||||
|
||||
### Required Fix
|
||||
|
||||
Imports must be placed **outside** the value wrapper:
|
||||
|
||||
```elixir
|
||||
import Enum # <- CORRECT: imports outside wrapper
|
||||
|
||||
result = (
|
||||
map([1,2,3], &(&1 * 2))
|
||||
)
|
||||
IO.puts(inspect(result, ...))
|
||||
```
|
||||
|
||||
## Fix Implementation Plan
|
||||
|
||||
### Overview
|
||||
|
||||
Instead of prepending imports to `full-body` before execution, we need to:
|
||||
1. Keep `full-body` without imports
|
||||
2. Pass `imports-string` as a separate parameter to execution functions
|
||||
3. Have execution functions prepend imports **after** the value wrapping occurs
|
||||
|
||||
### Step-by-Step Fix
|
||||
|
||||
#### Step 1: Revert org-babel-execute:elixir full-body calculation
|
||||
|
||||
**File:** `ob-elixir.el` (lines ~942-951)
|
||||
|
||||
**Current (BROKEN) code:**
|
||||
```elisp
|
||||
;; Find imports for this block's position
|
||||
(imports-string (ob-elixir--find-imports-for-position (point)))
|
||||
;; Expand body with variable assignments
|
||||
(full-body (let ((expanded (org-babel-expand-body:generic
|
||||
body params
|
||||
(org-babel-variable-assignments:elixir params))))
|
||||
;; Prepend imports if present
|
||||
(if imports-string
|
||||
(concat (string-trim imports-string) "\n\n" expanded)
|
||||
expanded)))
|
||||
```
|
||||
|
||||
**Change to:**
|
||||
```elisp
|
||||
;; Find imports for this block's position
|
||||
(imports-string (ob-elixir--find-imports-for-position (point)))
|
||||
;; Expand body with variable assignments
|
||||
(full-body (org-babel-expand-body:generic
|
||||
body params
|
||||
(org-babel-variable-assignments:elixir params)))
|
||||
```
|
||||
|
||||
**Rationale:** Don't prepend imports to full-body; instead pass imports-string separately.
|
||||
|
||||
---
|
||||
|
||||
#### Step 2: Update ob-elixir--execute to handle imports
|
||||
|
||||
**File:** `ob-elixir.el` (function starts at line ~907)
|
||||
|
||||
**Current code:**
|
||||
```elisp
|
||||
(defun ob-elixir--execute (body result-type)
|
||||
"Execute BODY as Elixir code.
|
||||
|
||||
RESULT-TYPE is either `value' or `output'.
|
||||
For `value', wraps code to capture return value.
|
||||
For `output', captures stdout directly.
|
||||
|
||||
Returns the result as a string.
|
||||
May signal `ob-elixir-error' if execution fails and
|
||||
`ob-elixir-signal-errors' is non-nil."
|
||||
(let* ((tmp-file (org-babel-temp-file "ob-elixir-" ".exs"))
|
||||
(code (if (eq result-type 'value)
|
||||
(ob-elixir--wrap-for-value body)
|
||||
body)))
|
||||
(with-temp-file tmp-file
|
||||
(insert code))
|
||||
(let ((result (with-temp-buffer
|
||||
(call-process ob-elixir-command nil t nil
|
||||
(org-babel-process-file-name tmp-file))
|
||||
;; Capture both stdout and stderr
|
||||
(buffer-string))))
|
||||
(ob-elixir--process-result result))))
|
||||
```
|
||||
|
||||
**Change to:**
|
||||
```elisp
|
||||
(defun ob-elixir--execute (body result-type &optional imports-string)
|
||||
"Execute BODY as Elixir code.
|
||||
|
||||
RESULT-TYPE is either `value' or `output'.
|
||||
For `value', wraps code to capture return value.
|
||||
For `output', captures stdout directly.
|
||||
IMPORTS-STRING, if provided, is prepended to the code before execution.
|
||||
|
||||
Returns the result as a string.
|
||||
May signal `ob-elixir-error' if execution fails and
|
||||
`ob-elixir-signal-errors' is non-nil."
|
||||
(let* ((tmp-file (org-babel-temp-file "ob-elixir-" ".exs"))
|
||||
(wrapped (if (eq result-type 'value)
|
||||
(ob-elixir--wrap-for-value body)
|
||||
body))
|
||||
(code (if imports-string
|
||||
(concat (string-trim imports-string) "\n\n" wrapped)
|
||||
wrapped)))
|
||||
(with-temp-file tmp-file
|
||||
(insert code))
|
||||
(let ((result (with-temp-buffer
|
||||
(call-process ob-elixir-command nil t nil
|
||||
(org-babel-process-file-name tmp-file))
|
||||
;; Capture both stdout and stderr
|
||||
(buffer-string))))
|
||||
(ob-elixir--process-result result))))
|
||||
```
|
||||
|
||||
**Key changes:**
|
||||
- Add `&optional imports-string` parameter
|
||||
- Rename `code` variable to `wrapped` (contains wrapped or unwrapped body)
|
||||
- Create new `code` that prepends imports to `wrapped` if imports-string present
|
||||
- Imports are now outside the value wrapper
|
||||
|
||||
---
|
||||
|
||||
#### Step 3: Update ob-elixir--execute-with-deps to handle imports
|
||||
|
||||
**File:** `ob-elixir.el` (function starts at line ~561)
|
||||
|
||||
**Current code:**
|
||||
```elisp
|
||||
(defun ob-elixir--execute-with-deps (body result-type deps-string)
|
||||
"Execute BODY with dependencies from DEPS-STRING.
|
||||
|
||||
RESULT-TYPE is `value' or `output'."
|
||||
(let* ((project-dir (ob-elixir--get-deps-project deps-string))
|
||||
(default-directory project-dir)
|
||||
(tmp-file (org-babel-temp-file "ob-elixir-" ".exs"))
|
||||
(code (if (eq result-type 'value)
|
||||
(ob-elixir--wrap-for-value body)
|
||||
body)))
|
||||
|
||||
;; Write code to temp file
|
||||
(with-temp-file tmp-file
|
||||
(insert code))
|
||||
|
||||
;; Execute with mix run
|
||||
(let ((command (format "%s run --no-compile %s 2>&1"
|
||||
ob-elixir-mix-command
|
||||
(shell-quote-argument tmp-file))))
|
||||
(ob-elixir--process-result
|
||||
(shell-command-to-string command)))))
|
||||
```
|
||||
|
||||
**Change to:**
|
||||
```elisp
|
||||
(defun ob-elixir--execute-with-deps (body result-type deps-string &optional imports-string)
|
||||
"Execute BODY with dependencies from DEPS-STRING.
|
||||
|
||||
RESULT-TYPE is `value' or `output'.
|
||||
IMPORTS-STRING, if provided, is prepended to the code before execution."
|
||||
(let* ((project-dir (ob-elixir--get-deps-project deps-string))
|
||||
(default-directory project-dir)
|
||||
(tmp-file (org-babel-temp-file "ob-elixir-" ".exs"))
|
||||
(wrapped (if (eq result-type 'value)
|
||||
(ob-elixir--wrap-for-value body)
|
||||
body))
|
||||
(code (if imports-string
|
||||
(concat (string-trim imports-string) "\n\n" wrapped)
|
||||
wrapped)))
|
||||
|
||||
;; Write code to temp file
|
||||
(with-temp-file tmp-file
|
||||
(insert code))
|
||||
|
||||
;; Execute with mix run
|
||||
(let ((command (format "%s run --no-compile %s 2>&1"
|
||||
ob-elixir-mix-command
|
||||
(shell-quote-argument tmp-file))))
|
||||
(ob-elixir--process-result
|
||||
(shell-command-to-string command)))))
|
||||
```
|
||||
|
||||
**Key changes:**
|
||||
- Add `&optional imports-string` parameter
|
||||
- Same wrapping logic as Step 2
|
||||
|
||||
---
|
||||
|
||||
#### Step 4: Update ob-elixir--evaluate-in-session to handle imports
|
||||
|
||||
**File:** `ob-elixir.el` (function starts at line ~681)
|
||||
|
||||
**Current code:**
|
||||
```elisp
|
||||
(defun ob-elixir--evaluate-in-session (session body result-type)
|
||||
"Evaluate BODY in SESSION, return result.
|
||||
|
||||
RESULT-TYPE is `value' or `output'."
|
||||
(let* ((buffer (org-babel-elixir-initiate-session session nil))
|
||||
(code (if (eq result-type 'value)
|
||||
(ob-elixir--session-wrap-for-value body)
|
||||
body))
|
||||
(eoe-indicator ob-elixir--eoe-marker)
|
||||
(full-body (concat code "\nIO.puts(\"" eoe-indicator "\")"))
|
||||
output)
|
||||
(unless buffer
|
||||
(error "Failed to create Elixir session: %s" session))
|
||||
|
||||
(setq output
|
||||
(org-babel-comint-with-output
|
||||
(buffer eoe-indicator t full-body)
|
||||
(ob-elixir--send-command buffer full-body)))
|
||||
|
||||
(ob-elixir--clean-session-output output result-type)))
|
||||
```
|
||||
|
||||
**Change to:**
|
||||
```elisp
|
||||
(defun ob-elixir--evaluate-in-session (session body result-type &optional imports-string)
|
||||
"Evaluate BODY in SESSION, return result.
|
||||
|
||||
RESULT-TYPE is `value' or `output'.
|
||||
IMPORTS-STRING, if provided, is prepended to the code before execution."
|
||||
(let* ((buffer (org-babel-elixir-initiate-session session nil))
|
||||
(wrapped (if (eq result-type 'value)
|
||||
(ob-elixir--session-wrap-for-value body)
|
||||
body))
|
||||
(code (if imports-string
|
||||
(concat (string-trim imports-string) "\n\n" wrapped)
|
||||
wrapped))
|
||||
(eoe-indicator ob-elixir--eoe-marker)
|
||||
(full-body (concat code "\nIO.puts(\"" eoe-indicator "\")"))
|
||||
output)
|
||||
(unless buffer
|
||||
(error "Failed to create Elixir session: %s" session))
|
||||
|
||||
(setq output
|
||||
(org-babel-comint-with-output
|
||||
(buffer eoe-indicator t full-body)
|
||||
(ob-elixir--send-command buffer full-body)))
|
||||
|
||||
(ob-elixir--clean-session-output output result-type)))
|
||||
```
|
||||
|
||||
**Key changes:**
|
||||
- Add `&optional imports-string` parameter
|
||||
- Rename `code` to `wrapped`
|
||||
- Create new `code` that prepends imports if present
|
||||
- Use `code` for `full-body` concatenation
|
||||
|
||||
---
|
||||
|
||||
#### Step 5: Update ob-elixir--evaluate-in-session-with-deps to handle imports
|
||||
|
||||
**File:** `ob-elixir.el` (function starts at line ~831)
|
||||
|
||||
**Current code:**
|
||||
```elisp
|
||||
(defun ob-elixir--evaluate-in-session-with-deps (session body result-type deps-string)
|
||||
"Evaluate BODY in SESSION with DEPS-STRING context.
|
||||
|
||||
RESULT-TYPE is `value' or `output'."
|
||||
(let* ((buffer (ob-elixir--get-or-create-session-with-deps session deps-string))
|
||||
(code (if (eq result-type 'value)
|
||||
(ob-elixir--session-wrap-for-value body)
|
||||
body))
|
||||
(eoe-indicator ob-elixir--eoe-marker)
|
||||
(full-body (concat code "\nIO.puts(\"" eoe-indicator "\")"))
|
||||
output)
|
||||
|
||||
(unless buffer
|
||||
(error "Failed to create Elixir session with deps: %s" session))
|
||||
|
||||
(setq output
|
||||
(org-babel-comint-with-output
|
||||
(buffer eoe-indicator t full-body)
|
||||
(ob-elixir--send-command buffer full-body)))
|
||||
|
||||
(ob-elixir--clean-session-output output result-type)))
|
||||
```
|
||||
|
||||
**Change to:**
|
||||
```elisp
|
||||
(defun ob-elixir--evaluate-in-session-with-deps (session body result-type deps-string &optional imports-string)
|
||||
"Evaluate BODY in SESSION with DEPS-STRING context.
|
||||
|
||||
RESULT-TYPE is `value' or `output'.
|
||||
IMPORTS-STRING, if provided, is prepended to the code before execution."
|
||||
(let* ((buffer (ob-elixir--get-or-create-session-with-deps session deps-string))
|
||||
(wrapped (if (eq result-type 'value)
|
||||
(ob-elixir--session-wrap-for-value body)
|
||||
body))
|
||||
(code (if imports-string
|
||||
(concat (string-trim imports-string) "\n\n" wrapped)
|
||||
wrapped))
|
||||
(eoe-indicator ob-elixir--eoe-marker)
|
||||
(full-body (concat code "\nIO.puts(\"" eoe-indicator "\")"))
|
||||
output)
|
||||
|
||||
(unless buffer
|
||||
(error "Failed to create Elixir session with deps: %s" session))
|
||||
|
||||
(setq output
|
||||
(org-babel-comint-with-output
|
||||
(buffer eoe-indicator t full-body)
|
||||
(ob-elixir--send-command buffer full-body)))
|
||||
|
||||
(ob-elixir--clean-session-output output result-type)))
|
||||
```
|
||||
|
||||
**Key changes:**
|
||||
- Add `&optional imports-string` parameter
|
||||
- Same wrapping logic as Step 4
|
||||
|
||||
---
|
||||
|
||||
#### Step 6: Update all function calls in org-babel-execute:elixir
|
||||
|
||||
**File:** `ob-elixir.el` (in `org-babel-execute:elixir`, around lines 952-966)
|
||||
|
||||
**Current code:**
|
||||
```elisp
|
||||
(result (condition-case err
|
||||
(cond
|
||||
;; Session mode with deps
|
||||
((and session (not (string= session "none")) deps-string)
|
||||
(ob-elixir--evaluate-in-session-with-deps
|
||||
session full-body result-type deps-string))
|
||||
;; Session mode without deps
|
||||
((and session (not (string= session "none")))
|
||||
(ob-elixir--evaluate-in-session session full-body result-type))
|
||||
;; Non-session with deps
|
||||
(deps-string
|
||||
(ob-elixir--execute-with-deps full-body result-type deps-string))
|
||||
;; Plain execution
|
||||
(t
|
||||
(ob-elixir--execute full-body result-type)))
|
||||
(ob-elixir-error
|
||||
;; Return error message so it appears in buffer
|
||||
(cadr err)))))
|
||||
```
|
||||
|
||||
**Change to:**
|
||||
```elisp
|
||||
(result (condition-case err
|
||||
(cond
|
||||
;; Session mode with deps
|
||||
((and session (not (string= session "none")) deps-string)
|
||||
(ob-elixir--evaluate-in-session-with-deps
|
||||
session full-body result-type deps-string imports-string))
|
||||
;; Session mode without deps
|
||||
((and session (not (string= session "none")))
|
||||
(ob-elixir--evaluate-in-session session full-body result-type imports-string))
|
||||
;; Non-session with deps
|
||||
(deps-string
|
||||
(ob-elixir--execute-with-deps full-body result-type deps-string imports-string))
|
||||
;; Plain execution
|
||||
(t
|
||||
(ob-elixir--execute full-body result-type imports-string)))
|
||||
(ob-elixir-error
|
||||
;; Return error message so it appears in buffer
|
||||
(cadr err)))))
|
||||
```
|
||||
|
||||
**Key changes:**
|
||||
- Add `imports-string` as final argument to all 4 function calls
|
||||
|
||||
---
|
||||
|
||||
#### Step 7: Run tests to verify
|
||||
|
||||
```bash
|
||||
make test
|
||||
```
|
||||
|
||||
All tests should now pass, including:
|
||||
- `ob-elixir-test-imports-execution`
|
||||
- `ob-elixir-test-imports-with-alias`
|
||||
|
||||
## Summary of Changes
|
||||
|
||||
| File | Line(s) | Change |
|
||||
|------|---------|--------|
|
||||
| `ob-elixir.el` | ~942-951 | Revert full-body calculation to not prepend imports |
|
||||
| `ob-elixir.el` | ~907-928 | Update `ob-elixir--execute` to accept and handle imports |
|
||||
| `ob-elixir.el` | ~561-581 | Update `ob-elixir--execute-with-deps` to accept and handle imports |
|
||||
| `ob-elixir.el` | ~681-700 | Update `ob-elixir--evaluate-in-session` to accept and handle imports |
|
||||
| `ob-elixir.el` | ~831-851 | Update `ob-elixir--evaluate-in-session-with-deps` to accept and handle imports |
|
||||
| `ob-elixir.el` | ~952-966 | Pass `imports-string` to all 4 execution function calls |
|
||||
|
||||
## Why This Fix Works
|
||||
|
||||
1. **Value wrapping happens first**: Body is wrapped with `result = (...)`
|
||||
2. **Imports added after wrapping**: Imports are prepended to the wrapped code
|
||||
3. **Result is valid Elixir**:
|
||||
```elixir
|
||||
import Enum # <- Outside expression (valid)
|
||||
|
||||
result = ( # <- Value wrapper
|
||||
map([1,2,3], &(&1 * 2))
|
||||
)
|
||||
IO.puts(inspect(result, ...))
|
||||
```
|
||||
|
||||
## Testing
|
||||
|
||||
The fix ensures that:
|
||||
- `ob-elixir-test-imports-execution` passes (tests `import Enum` with `map/2`)
|
||||
- `ob-elixir-test-imports-with-alias` passes (tests `alias String, as: S`)
|
||||
- All existing tests continue to pass
|
||||
- Imports work correctly in all modes: value, output, session, non-session, with/without deps
|
||||
Reference in New Issue
Block a user