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

[Ihor Radchenko <yantar92@posteo.net>]

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

> Let me know if you see further improvement before pushing this.

Thanks for the update!

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.

> From 9972b926f55cb970e0b520f8726a3684118017b6 Mon Sep 17 00:00:00 2001
> From: Ihor Radchenko <yantar92@posteo.net>
> Date: Fri, 24 Mar 2023 11:20:22 +0100
> Subject: [PATCH 02/13] org-babel-haskell-initiate-session: Remove secondary
>  prompt
>
> * lisp/ob-haskell.el (org-babel-haskell-initiate-session): Set
> secondary prompt to "".  If we do not do this, org-comint may treat
> secondary prompts as a part of output.

> +        (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.

> From 352d18399961fedc45cc2d64007016426e1ecd40 Mon Sep 17 00:00:00 2001
> From: Ihor Radchenko <yantar92@posteo.net>
> Date: Fri, 24 Mar 2023 11:26:00 +0100
> Subject: [PATCH 04/13] * testing/lisp/test-ob-haskell-ghci.el: Enable fixed

I do not see PATCH 03/13 in the attachments.

> From 7d66cff5cc23bb786cb2843f4326d2869512ccac Mon Sep 17 00:00:00 2001
> From: Bruno BARBIER <brubar.cs@gmail.com>
> Date: Sat, 25 Mar 2023 10:06:44 +0100
> Subject: [PATCH 06/13] ob-haskell: Implement sessions
>
> +  (unless session-name
> +    ;; As haskell-mode is using the buffer name "*haskell*", we stay
> +    ;; away from it.
> +    (setq session-name (generate-new-buffer-name "*ob-haskell*")))
> +  (let ((session (get-buffer session-name)))

session is not a buffer or nil, if no buffer named session-name exists.

> +    (save-window-excursion
> +      (or (org-babel-comint-buffer-livep session)

Below, (org-babel-comint-buffer-livep session) is nil, which implies
either that session is nil, does not exist, not live, or does not have a
process attached.

> +          (let ((inferior-haskell-buffer session))
> +            (when (and (bufferp session) (not (org-babel-comint-buffer-livep session)))

(not (org-babel-comint-buffer-livep session)) is always t here.
Also, session may be a killed buffer object. It is still a buffer, but
not usable. See `buffer-live-p'.

> +              (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.

> +            (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.

> +              (sleep-for 0.25)
> +              (setq session inferior-haskell-buffer)
> +              (with-current-buffer session (rename-buffer session-name))

This generally looks like a brittle workaround for inner workings of
haskell-mode. I recommend sending an email to haskell-mode devs,
requesting multiple session support. Otherwise, this whole code
eventually be broken.

> Subject: [PATCH 10/13] * testing/lisp/test-ob-haskell-ghci.el: Test output
>  without EOL
> ...
> +(ert-deftest ob-haskell/output-without-eol-1 ()
> +  "Cannot get output from incomplete lines, when entered line by line."
> +  :expected-result :failed
> +  (should (equal "123"
> +                 (test-ob-haskell-ghci ":results output" "
> +  putStr(\"1\")
> +  putStr(\"2\")
> +  putStr(\"3\")
> +  putStr(\"\\n\")
> +"))))

May you explain more about this bug?

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

> +   params
> +   (lambda (session)
> +     (cl-labels
> +         ((csend (txt)
> +          (eom ()
> +          (with-output (todo)

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.

> +              (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.

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

> Subject: [PATCH 12/13] * testing/lisp/test-ob-haskell-ghci.el: Modify
>  `test-ob-haskell-ghci`

Here and in some other patches you are undoing changes made in previous
patches. May you please consolidate transient changes by squashing
commits? It will make further reviews easier.

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