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