Hi Ihor,
Thanks for the review.
Ihor Radchenko writes:
> Bruno Barbier 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
>> 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
>> 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
>> 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 . 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,
