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.