From: Ihor Radchenko <yantar92@posteo.net>
To: Bruno Barbier <brubar.cs@gmail.com>
Cc: emacs-orgmode@gnu.org
Subject: Re: [PATCH] Add tests for ob-haskell (GHCi)
Date: Fri, 02 Jun 2023 08:44:24 +0000 [thread overview]
Message-ID: <87a5xi5k47.fsf@localhost> (raw)
In-Reply-To: <6469cafa.1c0a0220.4db6f.0396@mx.google.com>
[-- Attachment #1: Type: text/plain, Size: 4356 bytes --]
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.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: example.diff --]
[-- Type: text/x-patch, Size: 2444 bytes --]
diff --git a/lisp/ob-haskell.el b/lisp/ob-haskell.el
index cd930266c..a8240d02b 100644
--- a/lisp/ob-haskell.el
+++ b/lisp/ob-haskell.el
@@ -77,31 +77,28 @@ (defcustom org-babel-haskell-compiler "ghc"
(defconst org-babel-header-args:haskell '((compile . :any))
"Haskell-specific header arguments.")
-
-(defun org-babel-haskell-with-session--worker (params todo)
- "See `org-babel-haskell-with-session'."
- (let* ((sn (cdr (assq :session params)))
- (session (org-babel-haskell-initiate-session sn params))
- (one-shot (equal sn "none")))
- (unwind-protect
- (funcall todo session)
- (when (and one-shot (buffer-live-p session))
- ;; As we don't control how the session temporary buffer is
- ;; created, we need to explicitly work around the hooks and
- ;; query functions.
- (with-current-buffer session
- (let ((kill-buffer-query-functions nil)
- (kill-buffer-hook nil))
- (kill-buffer session)))))))
-
(defmacro org-babel-haskell-with-session (session-symbol params &rest body)
"Get the session identified by PARAMS and run BODY with it.
-Get or create a session, as needed to match PARAMS. Assign the session to
-SESSION-SYMBOL. Execute BODY. Destroy the session if needed.
-Return the value of the last form of BODY."
+Get or create a session, as needed to match PARAMS. Assign the
+session to SESSION-SYMBOL. Execute BODY. Destroy the session if
+needed. Return the value of the last form of BODY."
(declare (indent 2) (debug (symbolp form body)))
- `(org-babel-haskell-with-session--worker ,params (lambda (,session-symbol) ,@body)))
+ `(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)
+ (when (and one-shot (buffer-live-p session))
+ ;; As we don't control how the session temporary buffer is
+ ;; created, we need to explicitly work around the hooks and
+ ;; query functions.
+ (with-current-buffer session
+ (let ((kill-buffer-query-functions nil)
+ (kill-buffer-hook nil))
+ (kill-buffer session)))))))
(defun org-babel-haskell-execute (body params)
"This function should only be called by `org-babel-execute:haskell'."
[-- Attachment #3: Type: text/plain, Size: 1782 bytes --]
>> 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.
Yes. Thanks!
>>> + (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.
Well. You are right. Not that I like it. Let's leave this be until I
find time to refactor ob-core API.
>>> - (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.
--
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>
next prev parent reply other threads:[~2023-06-02 8:41 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-19 9:12 [PATCH] Add tests for ob-haskell (GHCi) Bruno Barbier
2023-03-19 10:20 ` Ihor Radchenko
2023-03-19 10:28 ` Ihor Radchenko
2023-03-19 10:32 ` Bruno Barbier
2023-03-22 10:16 ` Ihor Radchenko
2023-03-24 10:36 ` Ihor Radchenko
2023-03-25 10:01 ` Bruno Barbier
2023-03-26 9:09 ` Ihor Radchenko
2023-03-26 9:40 ` Bruno Barbier
2023-03-26 9:46 ` Ihor Radchenko
[not found] ` <notmuch-sha1-0807e1720f829950d42ef560bc30e56bd152766c>
2023-05-07 8:50 ` Bruno Barbier
2023-05-07 9:18 ` Ruijie Yu via General discussions about Org-mode.
2023-05-07 11:15 ` Bruno Barbier
2023-05-08 10:59 ` Ihor Radchenko
2023-05-21 7:40 ` Bruno Barbier
2023-06-02 8:44 ` Ihor Radchenko [this message]
2023-08-10 12:51 ` Ihor Radchenko
2023-08-25 19:10 ` Bruno Barbier
2023-09-07 14:21 ` Bruno Barbier
2023-09-08 8:23 ` Ihor Radchenko
2023-09-08 9:49 ` Bruno Barbier
2023-03-23 10:35 ` Ihor Radchenko
2023-03-23 21:01 ` ParetoOptimalDev
2023-03-23 21:30 ` ParetoOptimalDev
2023-03-24 10:40 ` Ihor Radchenko
2023-03-26 3:27 ` ParetoOptimalDev
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.orgmode.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87a5xi5k47.fsf@localhost \
--to=yantar92@posteo.net \
--cc=brubar.cs@gmail.com \
--cc=emacs-orgmode@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).