Re: [PATCH] Add tests for ob-haskell (GHCi)

[Ihor Radchenko]

Bruno Barbier <brubar.cs@gmail.com> writes:

>> I can see that you limited the tests scope to :session blocks.
>> Would it be possible to extend the existing tests to :compile yes case?
>> From a glance, it does not look like you need to change much - Haskell
>> behaviour should be similar with or without ghci.
>
> Except for one line expressions, GHCi inputs and haskell modules are
> two different things.  I doubt there will be much to share; that's why
> I've focused from the start on GHCi only.

Fair point. I thought that the differences are less significant.

>>> +        (sleep-for 0.25)
>>> +        ;; Disable secondary prompt.
>>
>> It would be useful to explain the purpose of disabling the secondary
>> prompt in the source code comment itself, not just in the commit
>> message. It will improve readability.
>
> Are you reviewing your own improvements ? :-)

Sure. Why not :3 Nobody guaranteed that my code is free of errors.

> Fixed. I've copied/pasted your explanation in the code.

Thanks!

>> Also, session may be a killed buffer object. It is still a buffer, but
>> not usable. See `buffer-live-p'.
>
> By construction, if 'session' is a buffer, then, it is a live buffer.

You are probably right. Even though killed buffer objects may appear in
Elisp (28.10 Killing Buffers), their buffer names should be nil, and thus
they cannot be returned by `get-buffer'.

>>> +              (when (bufferp "*haskell*") (error "Conflicting buffer 
>>> '*haskell*', rename it or kill it."))
>>> +              (with-current-buffer session (rename-buffer "*haskell*")))
>>
>> So, you are now renaming the unique session buffer back to "*haskell*".
>> And never rename it back to expected :session <value>. Users might be 
>> confused.
>
> I do rename it back once inf-haskell has initialized the buffer (after
> run-haskell in the last version).

A comment would help to clarify things for the readers.

>>> +            (save-window-excursion
>>> +              ;; We don't use `run-haskell' to not popup the buffer.
>>> +              ;; And we protect default-directory.
>>> +              (let ((default-directory default-directory))
>>> +                (inferior-haskell-start-process))
>>
>> This is a workaround for a nasty side effect of running
>> `inferior-haskell-start-process'. We should report this to haskell-mode
>> developers, leaving appropriate comment in the code.
>
> About 'run-haskell', I reverted my change: we're inside a
> 'save-window-excursion', which looks like the standard way to get rid
> of unwanted popups (like ob-shell does).
>
> About 'default-directory', I'm not sure. Maybe the side effect is done
> on purpose in inf-haskell.

I can see the haskell-mode overrides default-directory with
`inferior-haskell-root-dir', running ghci in that directory, if it is
non-nil. Even with your let binding, it is calling for trouble when
source block uses :dir header argument.

Maybe we can bind `inferior-haskell-root-dir' to `default-directory'
instead? `default-directory' is modified according to :dir by ob-core.el
when necessary.

> The function 'putStr' output the string without a newline on stdout
> (as opposed to the function putStrLn that does add a newline).
>
> So, in GHCi, entering:
>
>     putStr("4")
>     
> outputs "4" on stdout, then GHCi outputs the prompt, so we get:
>
>     4ghci> 
>
> In the end, 'org-babel-comint-with-output' gets this
>     1ghci> 2ghci> 3ghci> 
>     ghci> org-babel-haskell-eoe
>     ghci> ghci>
>     
> and filters out everything as being GHCi prompts and the EOE.
>
> I'm not really expecting this to be fixed; I just wanted to record the
> fact.

We actually might be able to deal with this if we change the prompt and
update comint-prompt-regexp to something more accurate.

>>> Subject: [PATCH 11/13] lisp/ob-haskell.el: Fix how to use sessions
>>>
>>> +  (org-babel-haskell-with-session
>>
>> This kind of names are usually dedicated to macro calls. But
>> `org-babel-haskell-with-session' is instead a function. I think a macro
>> will be better. And you will be able to get rid of unnecessary lambda.
>
> That looks kind of complicated just to avoid one lambda in one call.
> But, as I couldn't find a better name, I've translated it into a
> macro.

I think you misunderstood what I meant.
See the attached diff on top of your patches that simplifies things a
bit.

diff --git a/lisp/ob-haskell.el b/lisp/ob-haskell.el
index cd930266c..a8240d02b 100644
--- a/lisp/ob-haskell.el
+++ b/lisp/ob-haskell.el
@@ -77,31 +77,28 @@ (defcustom org-babel-haskell-compiler "ghc"
 (defconst org-babel-header-args:haskell '((compile . :any))
   "Haskell-specific header arguments.")
 
-
-(defun org-babel-haskell-with-session--worker (params todo)
-  "See `org-babel-haskell-with-session'."
-  (let* ((sn (cdr (assq :session params)))
-         (session (org-babel-haskell-initiate-session sn params))
-         (one-shot (equal sn "none")))
-    (unwind-protect
-        (funcall todo session)
-      (when (and one-shot (buffer-live-p session))
-        ;; As we don't control how the session temporary buffer is
-        ;; created, we need to explicitly work around the hooks and
-        ;; query functions.
-        (with-current-buffer session
-          (let ((kill-buffer-query-functions nil)
-                (kill-buffer-hook nil))
-            (kill-buffer session)))))))
-
 (defmacro org-babel-haskell-with-session (session-symbol params &rest body)
   "Get the session identified by PARAMS and run BODY with it.
 
-Get or create a session, as needed to match PARAMS.  Assign the session to
-SESSION-SYMBOL.  Execute BODY. Destroy the session if needed.
-Return the value of the last form of BODY."
+Get or create a session, as needed to match PARAMS.  Assign the
+session to SESSION-SYMBOL.  Execute BODY.  Destroy the session if
+needed.  Return the value of the last form of BODY."
   (declare (indent 2) (debug (symbolp form body)))
-  `(org-babel-haskell-with-session--worker ,params (lambda (,session-symbol) 
,@body)))
+  `(let* ((params ,params)
+          (sn (cdr (assq :session params)))
+          (session (org-babel-haskell-initiate-session sn params))
+          (,session-symbol session)
+          (one-shot (equal sn "none")))
+     (unwind-protect
+         (progn ,@body)
+       (when (and one-shot (buffer-live-p session))
+         ;; As we don't control how the session temporary buffer is
+         ;; created, we need to explicitly work around the hooks and
+         ;; query functions.
+         (with-current-buffer session
+           (let ((kill-buffer-query-functions nil)
+                 (kill-buffer-hook nil))
+             (kill-buffer session)))))))
 
 (defun org-babel-haskell-execute (body params)
   "This function should only be called by `org-babel-execute:haskell'."

>> When using `cl-labels', please prefer longer, more descriptive function
>> names. These functions do not have a docstring and I now am left
>> guessing and reading the function code repeatedly to understand the
>> usage.
>
> I tried to use more descriptive names. I hope it's easier to read now.

Yes. Thanks!

>>> +              (full-body (org-babel-expand-body:generic
>>> +                          body params
>>> +                          (org-babel-variable-assignments:haskell params)))
>>
>> I think we want `org-babel-expand-src-block' here instead of using
>> semi-internal ob-core.el parts.
>
> Are you sure about this ?  I didn't modify this part and I didn't see
> this function used in other backends.  I've also checked ob-python and
> ob-shell: they both use the same code as above.

Well. You are right. Not that I like it. Let's leave this be until I
find time to refactor ob-core API.

>>> -    (let ((buffer (org-babel-haskell-initiate-session session)))
>>> +    (let ((buffer (org-babel-haskell-initiate-session session params)))
>>
>> PARAMS argument is ignored by `org-babel-haskell-initiate-session'. I am
>> not sure why you are trying to pass it here.
>
> We have the PARAMS, and, org-babel-haskell-initiate-session has a
> PARAMS arguments. So, at the API level, I think it's better to
> propagate it than to ignore it.  But you're right that, today, the
> current implementation ignores it.
>
> I'm fine with dropping that change if you so prefer.

I am mostly neutral here. Slightly in favour of keeping things unchanged.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
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>