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

[Bruno Barbier]

Hi Ihor,

Sorry for the delay, thanks again for the ping.



Ihor Radchenko <yantar92@posteo.net> writes:

> Bruno Barbier <brubar.cs@gmail.com> writes:
>
>>>> +              (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.

Right. I've improved the comment. Thanks.


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




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

You may be right that we should use `inferior-haskell-root-dir' to
tell haskell-mode where to run the interpreter.  Done.



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

I couldn't make it work by simply restricting the prompt.  I think
it's OK if character by character output doesn't work; ghci is about
line based interaction afterall.



>>>> 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
...
>  (defmacro org-babel-haskell-with-session (session-symbol params &rest body)
>    "Get the session identified by PARAMS and run BODY with it.
.. 
> +  `(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)

I don't think it's correct to create local variables like this in a
macro: we need to use uninterned symbols, else we may capture caller
variables (params, sn, session and one-shot).

I personnaly find it easier when I keep my macros as short as
possible, and, to do any non-trivial work in a function: easier to
read, to modify and to debug.




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

I've removed my change. The function `org-babel-load-session:haskell`
now ignores the parameter PARAMS, as before.


Thanks 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