From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp12.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms9.migadu.com with LMTPS id +OSlCSireWSiJgAASxT56A (envelope-from ) for ; Fri, 02 Jun 2023 10:41:12 +0200 Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp12.migadu.com with LMTPS id yM+VCSireWTLtwAAauVa8A (envelope-from ) for ; Fri, 02 Jun 2023 10:41:12 +0200 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id 1E8A81528C for ; Fri, 2 Jun 2023 10:41:11 +0200 (CEST) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1q50KP-0005eT-LC; Fri, 02 Jun 2023 04:40:09 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1q50KM-0005Yq-RW for emacs-orgmode@gnu.org; Fri, 02 Jun 2023 04:40:06 -0400 Received: from mout02.posteo.de ([185.67.36.66]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1q50KK-0007EV-05 for emacs-orgmode@gnu.org; Fri, 02 Jun 2023 04:40:06 -0400 Received: from submission (posteo.de [185.67.36.169]) by mout02.posteo.de (Postfix) with ESMTPS id 96E93240103 for ; Fri, 2 Jun 2023 10:39:57 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017; t=1685695197; bh=leBM/6jRQKNbI1Vh7xMyjRkKGbGGIp0mZgP0ExsJg/c=; h=From:To:Cc:Subject:Date:Message-ID:MIME-Version:From; b=rWExHFQvTR0uPG8MX3xyIrNjgAoQp092vsz0n2g1G/01afwS/A2hvFAW/alhwX7vV cW0xE+TyLZcjjbCA7fyxiWJgd0CBBFMPa+Z7qMOB9TyrQ0k8IEvNuluJ72zuIctbzP EtJmmFBF75o1ywf8PIGARZyLzdn8T6hcTGdqQFOLvynQ0F1WWH+IhjN5Zv3FlH7oUE SmYTwDuxiIR3QvsuhnUttfC3SGBMR2S8G7qAXSs0jVQUAwwQmV3cyeVGTRQbiKjNeA D0K4TUieMZvR2G/CGdBDiQDKYAhQ7+mBuojHE1i6DjnLiwXXnYkHJ33SnCJ2natYvD McXuz8gT2Spnw== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4QXbzK0Cvtz6trm; Fri, 2 Jun 2023 10:39:56 +0200 (CEST) From: Ihor Radchenko To: Bruno Barbier Cc: emacs-orgmode@gnu.org Subject: Re: [PATCH] Add tests for ob-haskell (GHCi) In-Reply-To: <6469cafa.1c0a0220.4db6f.0396@mx.google.com> References: <6416d214.5d0a0220.d9c1.54aa@mx.google.com> <875yaxvy93.fsf@localhost> <6416e4d9.df0a0220.ce03d.5c4b@mx.google.com> <87h6udozvh.fsf@localhost> <87r0texwor.fsf@localhost> <641ec686.050a0220.16dbc.a6ca@mx.google.com> <87fs9rvpyj.fsf@localhost> <6457663a.df0a0220.f6b38.a05e@mx.google.com> <873547ks1t.fsf@localhost> <6469cafa.1c0a0220.4db6f.0396@mx.google.com> Date: Fri, 02 Jun 2023 08:44:24 +0000 Message-ID: <87a5xi5k47.fsf@localhost> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Received-SPF: pass client-ip=185.67.36.66; envelope-from=yantar92@posteo.net; helo=mout02.posteo.de X-Spam_score_int: -43 X-Spam_score: -4.4 X-Spam_bar: ---- X-Spam_report: (-4.4 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_MED=-2.3, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-orgmode@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+larch=yhetil.org@gnu.org Sender: emacs-orgmode-bounces+larch=yhetil.org@gnu.org X-Migadu-Country: US X-Migadu-Flow: FLOW_IN ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=yhetil.org; s=key1; t=1685695271; h=from:from:sender:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:in-reply-to:in-reply-to: references:references:list-id:list-help:list-unsubscribe: list-subscribe:list-post:dkim-signature; bh=H8oYHE+p+4KRRA2xUGYJhaHhbZLZ4QvtATysiuNOKv0=; b=apJg2YFXKrtvnUQsQI6IPPEUnXmodcQG0cbOb/slKmU+sagSG2NG1ivm3tf8o0+V9r07Am MOWW4VDaahYp4V8aXN9C9NRw+Nc0DHq86Qqhh26rEizLaTG1MksOhcf1ueNmu4otLMaGLg WQWuW8DSphKaUctACxWBNPflITZ62MzlCqvTcXnJ1pj9DhW80cGrRkbH4Lgp1ZzQiYsrMD y7PZt60yTivF+Fb+AuF1nuXi+Jhe6zlDUELU5SK6f1qyVHUHP9xix3gNgaLBX1h2Mq5qqX uRTxnxJUrGeDxD7xjwdsadp0qyLf+nA94ctKhVV0zuOOyWnoHSGURlk2h6++kw== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=pass header.d=posteo.net header.s=2017 header.b=rWExHFQv; dmarc=pass (policy=none) header.from=posteo.net; spf=pass (aspmx1.migadu.com: domain of "emacs-orgmode-bounces+larch=yhetil.org@gnu.org" designates 209.51.188.17 as permitted sender) smtp.mailfrom="emacs-orgmode-bounces+larch=yhetil.org@gnu.org" ARC-Seal: i=1; s=key1; d=yhetil.org; t=1685695271; a=rsa-sha256; cv=none; b=EOmnSlqonXCOZaqx92DaJjCiYjA8mzN5X8ktYR52e9xSZPPb4K8LgTN5/ll25AK8sNgxws hGFBiZz5m1KyZtwBlVuyhil5C3mDdSDbk+FttvdyIH1S76t+z5h+QfFnss8UunJBclCYGC 3ZxocI23EXKLIG7nNnsoFnvKS3QNykIMDPFQx/f/cnjc0nE7KmgH8RD4zPp8tFQPcB0422 ETy0y8+NuDOOqlSdX1eUZ+ck5SHqm1dlu0Qvl2JkQBBBGG3mghOd1iVQQ787877fSLRdWg Z4KeUlzQhUg6YncFkDEBq0XEQfRuvHHeSrH5iBJAGwcggOWbyWko7R9pKUWcew== X-Migadu-Scanner: scn1.migadu.com X-Migadu-Spam-Score: -6.13 Authentication-Results: aspmx1.migadu.com; dkim=pass header.d=posteo.net header.s=2017 header.b=rWExHFQv; dmarc=pass (policy=none) header.from=posteo.net; spf=pass (aspmx1.migadu.com: domain of "emacs-orgmode-bounces+larch=yhetil.org@gnu.org" designates 209.51.188.17 as permitted sender) smtp.mailfrom="emacs-orgmode-bounces+larch=yhetil.org@gnu.org" X-Migadu-Queue-Id: 1E8A81528C X-Spam-Score: -6.13 X-TUID: ydKtRw7V1ulw --=-=-= Content-Type: text/plain 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. 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 . 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. --=-=-= Content-Type: text/x-patch Content-Disposition: inline; filename=example.diff 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'." --=-=-= Content-Type: text/plain >> 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 . Support Org development at , or support my work at --=-=-=--