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

[Bruno Barbier]

Hi Ihor,

Thanks for the review.

    
Ihor Radchenko <yantar92@posteo.net> writes:
> 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.

As I've a lot of other things that I'd like to do to improve my day to
day workflow, and as I'm barely using ob-haskell, I can't promise I'll
work on this any time soon.


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

Are you reviewing your own improvements ? :-)

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



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

Sorry. I forgot it the first time. The email, after handling the
comments from Ruijie Yu, had it though.



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

The argument SESSION-NAME must be a string or nil (I added a test to
make clear that it must be a string).  Thus, session will either be nil or
a live buffer.



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

ok. So, in our case, session is either nil, or it's a live buffer
without an attached process.

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

Right. I removed the test. 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.


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


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



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

Yes. It's a workaround.  But it looks reasonably safe to me, as the
default buffer name isn't going to change, even if they make it
configurable.  Plus, 'make-comint' (used by the function
'inferior-haskell-start-process' from the library inf-haskell) would
also require to rename the buffer, or to forbid session names that
don't start and end with "*", or to use buffer names that don't match
the session names.



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

Sure.

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.

IMHO, users should use one of the alternatives, that are shown in the
tests 'ob-haskell/output-without-eol-2' or
'ob-haskell/output-without-eol-3'.


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

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

I tried to use more descriptive names. I hope it's easier to read now.



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



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


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

I wasn't sure what would make the review easier: keep the history
of changes or squash my updates.  I've now squashed all my updates.

I've attached the new version.

Thanks again for the review.

Bruno.

0001-ob-haskell-Add-tests-for-GHCi.patch
Description: Text Data

0002-org-babel-haskell-initiate-session-Remove-secondary-.patch
Description: Text Data

0003-testing-lisp-test-ob-haskell-ghci.el-Fix-some-tests.patch
Description: Text Data

0004-testing-lisp-test-ob-haskell-ghci.el-Enable-fixed-te.patch
Description: Text Data

0005-lisp-ob-haskell-Request-the-last-value-from-GHCi.patch
Description: Text Data

0006-ob-haskell-Implement-sessions.patch
Description: Text Data

0007-testing-lisp-test-ob-haskell-ghci.el-Test-output-wit.patch
Description: Text Data