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>
next prev parent 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
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=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 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).