all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Ihor Radchenko <yantar92@posteo.net>
To: Matt <matt@excalamus.com>, Bastien <bzg@gnu.org>
Cc: emacs-orgmode <emacs-orgmode@gnu.org>
Subject: Re: ob-shell intentions and paperwork (was Bash results broken?)
Date: Thu, 29 Dec 2022 11:08:59 +0000	[thread overview]
Message-ID: <87edsiv54k.fsf@localhost> (raw)
In-Reply-To: <18555580d75.d7b471f9370716.1497263973038999899@excalamus.com>

Bastien, could you please check Matt's copyright paperwork record in
FSF?

Matt <matt@excalamus.com> writes:

>  ---- On Wed, 21 Dec 2022 01:17:50 -0500  Matt  wrote --- 
>
>  > Currently, though, I'm refactoring the ob-shell tests to remove dependency on ob-shell-test.org and to stop the suite from littering. 
>
> Done.  Branched off bugfix, 12e10eb0d, and refactored test-ob-shell.el.  See attached patches.

Better use main for development.
bugfix is for... bugfixes :)

>  >  After that, I intend to incorporate the worg examples as tests.
>
> This is probably a good place for me to pause and get feedback before writing more code. 

Sure. See below.

> I just got the signed paperwork back from Craig and am waiting to be admitted to the Emacs group on Savannah.

Does it mean that you are willing to maintain lisp/ob-shell.el?
We usually give write access to the maintainers and regular
contributors. AFAIR, you previously contributed to WORG but not to Org
core.

In order for you to get admitted to Emacs group, we will first need to
reach out to Emacs maintainers authorizing your write access.

Meanwhile, once Bastien confirms your copyright status, I can apply
patches on your behalf.

> From ecdb71afa00ca137b4faa737393cb027907a7f9f Mon Sep 17 00:00:00 2001
> From: Matt Trzcinski <matt@excalamus.com>
> Date: Tue, 20 Dec 2022 13:51:26 -0500
> Subject: [PATCH 01/20] test-ob-shell.el: Remove mixed tabs and spaces
>
> * test-ob-shell.el: Convert tabs to spaces.
>
> Change made under the premise that one or the other is better than
> both.

We generally avoid whitespace-only changes. They tend to generate merge
conflicts. We do whitespace changes alongside with other changes in the
code only - this minimizes merge conflicts.
See https://orgmode.org/list/874jvkz5k8.fsf@bzg.fr

>  (ert-deftest test-ob-shell/dont-error-on-empty-results ()
> -  "Was throwing an elisp error when shell blocks threw errors and
> -returned empty results."
> -  (should (null (org-babel-execute:sh "ls NoSuchFileOrDirectory.txt" nil))))
> +  (should (null (org-babel-execute:sh nil nil))))

nil BODY argument for `org-babel-execute:sh' is not supported.
Better use explicit empty body: "".

It is also a good idea to add docstrings to the tests for completeness.

> [fn:1] Session name is a string and not a variable (such as
> `session-name').  This is because `org-babel-execute:sh' produces a
> type error when the session name is defined with a variable.  The
> source of the error appears to be the `params' symbol in
> `org-babel-execute:sh'.  `params' does not evaluate as a variable as
> expected–it evaluates as a symbol.  However, `org-babel-execute:sh' is
> defined within a function factory which makes it difficult to debug.
> Hard code the test name for now and refactor it later once
> `org-babel-execute:sh` is refactored.

There is nothing wrong here. `org-babel-execute-src-block' takes care
about parameter processing making sure that :session value is always a
string.

> +;; TODO refactor into macro.  Currently violates (elisp) Coding
> +;; Conventions and is hard to debug.
>  (defun org-babel-shell-initialize ()
>    "Define execution functions associated to shell names.

Could you please elaborate? Which particular convention does it violate?
What is hard to debug?

>  (ert-deftest test-ob-shell/dont-insert-spaces-on-expanded-bodies ()
>    "Expanded shell bodies should not start with a blank line unless
> -the body of the tangled block does."
> +the strings of the tangled block does."

What does "strings of the tangled block" refer to? The previous variant
is a lot more clear for me.

> +(ert-deftest test-ob-shell/generic-uses-no-arrays ()
> +  "Test generic serialization of array into a single string."
> +  (org-test-with-temp-text
> +      (test-ob-shell-multiline-string
> +       "#+NAME: sample_array"
> +       "| one   |"
> +       "| two   |"
> +       "| three |"

Why do you need `test-ob-shell-multiline-string' here?
Can simply type-in the string directly, as the rest of tests do.

> +(require 'org-test (expand-file-name "../org-test.el"))

I am unsure here. What will happen if you run this file from
default-directory not the same with file location?

Also, the repetitive patches changing names + killing error buffer for
individual tests can be merged into a single patch.

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


  reply	other threads:[~2022-12-29 11:10 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-16 16:22 Bash results broken? Rudolf Adamkovič
2022-12-16 17:41 ` Ihor Radchenko
2022-12-18 11:19   ` Ihor Radchenko
2022-12-20  0:44     ` Rudolf Adamkovič
2022-12-20  3:40       ` Matt
2022-12-25 11:23         ` Ihor Radchenko
2022-12-26  2:25           ` Matt
2022-12-26  9:26             ` Ihor Radchenko
2022-12-21  6:17   ` ob-shell intentions and paperwork (was Bash results broken?) Matt
2022-12-27 20:48     ` Matt
2022-12-29 11:08       ` Ihor Radchenko [this message]
2022-12-29 14:20         ` Bastien Guerry
2022-12-30  5:34         ` Matt
2022-12-30  8:06           ` Bastien Guerry
2022-12-30 18:46           ` Matt
2022-12-31 14:31             ` Ihor Radchenko
2023-01-01 23:55               ` Matt
2023-01-02  9:47                 ` Ihor Radchenko
2023-01-02 16:40                   ` Matt
2023-01-03 10:50                     ` Ihor Radchenko
2023-01-03 13:00                       ` Matt
2023-01-05 10:31                         ` Ihor Radchenko
2023-01-05 11:21                           ` Bastien Guerry
2023-01-10  2:31                             ` Matt
2023-01-11 11:53                               ` Ihor Radchenko
2023-01-11 16:18                                 ` Matt
2023-01-11 17:02                                   ` Ihor Radchenko
2023-01-11 19:34                                     ` Matt
2023-01-12  8:26                                       ` Ihor Radchenko
2023-01-12 14:43                                     ` Max Nikulin
2023-01-13  9:36                                       ` Ihor Radchenko
2023-01-13 15:18                                         ` Matt
2023-01-13 15:23                                           ` Ihor Radchenko
2023-01-14  7:41                                             ` Max Nikulin
2023-01-14 10:35                                               ` Ihor Radchenko
2023-01-14 15:09                                                 ` cgit and merge commit Max Nikulin
2023-01-24 20:16                                                   ` Ihor Radchenko
2022-12-31 12:56           ` ob-shell intentions and paperwork (was Bash results broken?) Ihor Radchenko
2023-01-02  4:40             ` Refactor org-babel-shell-initialize? (was Re: ob-shell intentions and paperwork (was Bash results broken?)) Matt
2023-01-03  9:29               ` Ihor Radchenko
2023-01-05  8:32                 ` Ihor Radchenko

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

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87edsiv54k.fsf@localhost \
    --to=yantar92@posteo.net \
    --cc=bzg@gnu.org \
    --cc=emacs-orgmode@gnu.org \
    --cc=matt@excalamus.com \
    /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 external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.