Re: [PATCH] ob-shell-test, test-ob-shell and introduction

[Max Nikulin]

On 06/01/2022 10:47, Matt wrote:


  > - In your examples variable values are simple. Often it is safer to add
  > double quotes around variable or command expansion. I would consider
  > adding quotes just to encourage people to do the same by default with

I appreciate these clarifications. Admittedly, I'm not great with
shell scripting.  Your recommendations appear sound to me and I've tried
to incorporate them.


I think, to illustrate unset variable escaped quotes were appropriate. 
Sorry that I was not clear enough.


   echo "X was set to \"$X\""

One more case where it is better (at least from my point of view) to add 
quotes even though they are not strictly necessary since command output 
contains single word and multiple words are interpreted by echo in the 
same way:


   echo "$(cut -f 1 -d "/") rocks!"


The updates have been made and are pushed. Thanks for your feedback!


Thank you, text is more clear now. However first time I read it with 
more attention.


I forgot to suggest you the following tool that catches more problems 
than execution the script with "-n" option:


echo 'a=2; echo "$(($a * 2))"' | shellcheck -s sh -

In - line 1:
a=2; echo "$(($a * 2))"
  ^-- SC2004: $/${} is unnecessary on arithmetic variables.

For more information:
  https://www.shellcheck.net/wiki/SC2004 -- $/${} is unnecessary on 
arithmeti...

Re: [PATCH] ob-shell-test, test-ob-shell and introduction

[Matt]

 > Thank you, I was not aware of ":cmdline" argument and of ":shebang" as a 
 > means to avoid stray prompts (I have seen mentions of similar problem 
 > for other languages in this list).
 
You're welcome! I was surprised to find those in the source. Glad to have 
documented them and even happier to hear that it's news to others.
 
 > - https://www.mirbsd.org/pics/logo-grey.png works only with http: 
 > protocol for me.

I updated (downgraded?) it to http.  I'm also seeing that the kornshell logo is 
intermittently broken.  Not quite sure the best way to go about fixing that.   
Maybe commit copies?  Use something like archive.org? Sounds like a problem for 
someone else (e.g. future me).  I'm out of time for tonight.

 > - I am unsure concerning example after "…or as a standlone script.", 
 > maybe it should be wrapped into #+begin_example.

Good call.  That example was wonky.  I've given a different one.  Not sure it's 
better, though.  I find the shebang behavior tricky to demo in a simple way.

 > - In some cases "sh" is used despite bashisms in the code like "declare" 
 > or "echo -ne". Actually "printf" may be more convenient instead of "echo -n"
 > - "export" in session example is unnecessary. It is matter of taste 
 > though. Bash and dash is not the case, but some shells require that 
 > assignment and export should be separate commands.

Good catch!  Since arrays are only defined in ob-shell using bash, I've updated 
that example to use bash. Otherwise, I've removed the exports. 

 > - In your examples variable values are simple. Often it is safer to add 
 > double quotes around variable or command expansion. I would consider 
 > adding quotes just to encourage people to do the same by default with 
 > hope that less scripts will give strange results in response to a file 
 > name containing a space. Actually my first impression was that 
 > backslashes before quotes in some cases were added by mistake. Another 
 > unescaped pair of quotes may make your intention more clear. However it 
 > is related to code style where everybody has opinion, so I do not insist.

I appreciate these clarifications. Admittedly, I'm not great with shell 
scripting.  Your recommendations appear sound to me and I've tried to 
incorporate them.

The updates have been made and are pushed. Thanks for your feedback!

Re: [PATCH] ob-shell-test, test-ob-shell and introduction

[Max Nikulin]

On 01/01/2022 02:18, Matt wrote:


  > Wow.  Nice work!

Thanks. I pushed things to Worg, if you haven't seen already
(https://orgmode.org/worg/org-contrib/babel/languages/ob-doc-shell.html).
If you read it and find anything missing or unclear, please let me
know.


Thank you, I was not aware of ":cmdline" argument and of ":shebang" as a 
means to avoid stray prompts (I have seen mentions of similar problem 
for other languages in this list).


I have noticed some glitches.

- https://www.mirbsd.org/pics/logo-grey.png works only with http: 
protocol for me.
- I am unsure concerning example after "…or as a standlone script.", 
maybe it should be wrapped into #+begin_example.
- In some cases "sh" is used despite bashisms in the code like "declare" 
or "echo -ne". Actually "printf" may be more convenient instead of "echo -n"
- "export" in session example is unnecessary. It is matter of taste 
though. Bash and dash is not the case, but some shells require that 
assignment and export should be separate commands.
- In your examples variable values are simple. Often it is safer to add 
double quotes around variable or command expansion. I would consider 
adding quotes just to encourage people to do the same by default with 
hope that less scripts will give strange results in response to a file 
name containing a space. Actually my first impression was that 
backslashes before quotes in some cases were added by mistake. Another 
unescaped pair of quotes may make your intention more clear. However it 
is related to code style where everybody has opinion, so I do not insist.

Re: [PATCH] ob-shell-test, test-ob-shell and introduction

[Thomas S. Dye]

Aloha all,

FWIW, as a user actively pursuing reproducible research with Org 
and a contributor to documentation about Org and Babel intended 
for other users (rather than Org mode elisp coders) I'd be pleased 
if Org's code custodians look favorably on this proposal.


+1

All the best,
Tom

Matt  writes:

Apologies for the book.  I've been sitting on this stuff for two 
months and am wondering how to proceed.


IANAL but AFAIK/CT, my contract contains an exception for making 
contributions to projects like Org. I've gotten confirmation 
from my manager and by HR.  However, until the CEO signs the FSF 
disclaimer, I can't officially contribute.  I'm confident that I 
can publish changes (e.g. to a personal website); the FSF just 
can't accept my changes (yet).


I could start working on ob-shell.el separately now and publish 
that myself. It's not clear how I would then contribute those 
changes back once I'm cleared by the FSF.  I'm inclined towards 
some refactoring and I'm not sure how I could break that down in 
such a way that if it took two more months to get the copyright 
stuff worked out that anyone could make sense of the changes.  I 
would much prefer to gradually submit patches, discuss them, and 
then eventually have them merged in turn.  If I have a heap of 
changes elsewhere, I feel like it would be harder to have a 
conversion about them.


Regardless, I think I should define test cases first.  If those 
are considered valid, then any refactoring would be moot if they 
pass, right?  With (agreed upon) test cases, it shouldn't matter 
if I refactor as long as functionality remains the same?


Overall, what advice do you have?

It looks to me like ob-shell.el could use some love in other 
respects besides async evaluation.  I've detailed where below, 
partly for my own organization and partly for posterity, but 
mainly because this isn't my house, so to speak, and I don't 
want to barge in and start rearranging the furniture and eating 
whatever's in the fridge.  Or, is it like Worg in that once I 
have the keys I can drive where I like, so long as there're no 
crashes?


I'm interested in people's thoughts on these notes on 
ob-shell.el:


* Tests
There are several code paths, like shebang, cmdline, and basic 
execution, which don't always have something to do with one 
another in the code.  Having tests would be really helpful to 
make sure everything jives.  Before doing anything with the code 
base, I intend to create tests for all the functionality.


* 2D-array
I documented two options for the =:var= header[fn:1]. The 
ob-shell.el code actually defines a third option for 2D-arrays. 
I couldn't get it to work.  It was one of several things not 
documented anywhere, at least as far as I could find, and which 
I had to figure out straight from the code.  Between not being 
great at shell scripting and having a hard time deciphering that 
ob-shell.el code, I'm not sure 2D-arrays are actually fully or 
correctly implemented.


* M-up behavior <>
The =org-babel-load-session:shell= code path only works when 
M-up is used on a code block[fn:2]. Is M-up actually documented 
anywhere?  Furthermore, the =org-babel-load-session:shell= only 
works for the "shell" language, which is not actually a "proper" 
shell (i.e. it's not listed in =org-babel-shell-names=). The 
M-up defaults to using $ESHELL or shell-file-name through the 
=shell= command.


For example, try calling M-up on these:

#+comment: (opaquely) calls the system shell
#+begin_src shell :session my-shell
echo "hello, world!"  #+end_src

#+comment: fails
#+begin_src sh :session my-sh
echo "hello, world!"  #+end_src

#+comment: fails
#+begin_src bash :session my-bash
echo "hello, world!"  #+end_src

To fix this, there needs to be an 
=org-babel-load-session:= for each language in 
=org-babel-shell-names=.  This would probably make the most 
sense in =org-babel-shell-initialize=.  However, that function 
[[org-babel-shell-initialize][needs attention]].


* Refactoring <>
The ob-shell.el code appears inconsistent to me.  Clearly, this 
is somewhat subjective.  I've tried to give a rationale for each 
point to make it less so.  My goal is to be maintainer of 
ob-shell.el, but that's not forever.  These things were an 
obstacle for me and my aim is to remove them for the next 
person.


** =org-babel-shell-initialize= <>
As alluded to elsewhere, =org-babel-shell-initialize= doesn't 
appear to adhere to the (elisp) Coding Conventions,


#+begin_quote
   • Constructs that define a function or variable should be 
   macros, not functions, and their names should start with 
   ‘define-’.  The macro should receive the name to be defined 
   as the first argument.  That will help various tools find the 
   definition automatically.  Avoid constructing the names in 
   the macro itself, since that would confuse these tools. 
   #+end_quote


The =org-babel-shell-initialize= function defines 
=org-babel-execute:=, 

Re: [PATCH] ob-shell-test, test-ob-shell and introduction

[Matt]

Apologies for the book.  I've been sitting on this stuff for two months and am 
wondering how to proceed.

IANAL but AFAIK/CT, my contract contains an exception for making contributions 
to projects like Org. I've gotten confirmation from my manager and by HR.  
However, until the CEO signs the FSF disclaimer, I can't officially contribute. 
 I'm confident that I can publish changes (e.g. to a personal website); the FSF 
just can't accept my changes (yet).

I could start working on ob-shell.el separately now and publish that myself. 
It's not clear how I would then contribute those changes back once I'm cleared 
by the FSF.  I'm inclined towards some refactoring and I'm not sure how I could 
break that down in such a way that if it took two more months to get the 
copyright stuff worked out that anyone could make sense of the changes.  I 
would much prefer to gradually submit patches, discuss them, and then 
eventually have them merged in turn.  If I have a heap of changes elsewhere, I 
feel like it would be harder to have a conversion about them.

Regardless, I think I should define test cases first.  If those are considered 
valid, then any refactoring would be moot if they pass, right?  With (agreed 
upon) test cases, it shouldn't matter if I refactor as long as functionality 
remains the same?

Overall, what advice do you have?

It looks to me like ob-shell.el could use some love in other respects besides 
async evaluation.  I've detailed where below, partly for my own organization 
and partly for posterity, but mainly because this isn't my house, so to speak, 
and I don't want to barge in and start rearranging the furniture and eating 
whatever's in the fridge.  Or, is it like Worg in that once I have the keys I 
can drive where I like, so long as there're no crashes?

I'm interested in people's thoughts on these notes on ob-shell.el:

* Tests
There are several code paths, like shebang, cmdline, and basic execution, which 
don't always have something to do with one another in the code.  Having tests 
would be really helpful to make sure everything jives.  Before doing anything 
with the code base, I intend to create tests for all the functionality.

* 2D-array
I documented two options for the =:var= header[fn:1]. The ob-shell.el code 
actually defines a third option for 2D-arrays.  I couldn't get it to work.  It 
was one of several things not documented anywhere, at least as far as I could 
find, and which I had to figure out straight from the code.  Between not being 
great at shell scripting and having a hard time deciphering that ob-shell.el 
code, I'm not sure 2D-arrays are actually fully or correctly implemented.

* M-up behavior <>
The =org-babel-load-session:shell= code path only works when M-up is used on a 
code block[fn:2]. Is M-up actually documented anywhere?  Furthermore, the 
=org-babel-load-session:shell= only works for the "shell" language, which is 
not actually a "proper" shell (i.e. it's not listed in 
=org-babel-shell-names=). The M-up defaults to using $ESHELL or shell-file-name 
through the =shell= command.

For example, try calling M-up on these:

#+comment: (opaquely) calls the system shell
#+begin_src shell :session my-shell
echo "hello, world!"  #+end_src

#+comment: fails
#+begin_src sh :session my-sh
echo "hello, world!"  #+end_src

#+comment: fails
#+begin_src bash :session my-bash
echo "hello, world!"  #+end_src

To fix this, there needs to be an =org-babel-load-session:= for each 
language in =org-babel-shell-names=.  This would probably make the most sense 
in =org-babel-shell-initialize=.  However, that function 
[[org-babel-shell-initialize][needs attention]].

* Refactoring <>
The ob-shell.el code appears inconsistent to me.  Clearly, this is somewhat 
subjective.  I've tried to give a rationale for each point to make it less so.  
My goal is to be maintainer of ob-shell.el, but that's not forever.  These 
things were an obstacle for me and my aim is to remove them for the next person.

** =org-babel-shell-initialize= <>
As alluded to elsewhere, =org-babel-shell-initialize= doesn't appear to adhere 
to the (elisp) Coding Conventions,

#+begin_quote
   • Constructs that define a function or variable should be macros, not 
functions, and their names should start with ‘define-’.  The macro should 
receive the name to be defined as the first argument.  That will help various 
tools find the definition automatically.  Avoid constructing the names in the 
macro itself, since that would confuse these tools.  #+end_quote

The =org-babel-shell-initialize= function defines =org-babel-execute:=, 
=org-babel-variable-assignments:=, and 
=org-babel-default-header-args:= for the "languages" in 
=org-babel-shell-names=.  As it stands, that =org-babel-shell-initialize= is a 
function does no harm (aside from being confusing by way of straying from 
convention).  However, if the [[M-up][M-up]] issue is to be resolved, it seems 
to me that =org-babel-shell-initialize= should be 

Re: [PATCH] ob-shell-test, test-ob-shell and introduction

[Thomas S. Dye]

Aloha Matt,

Matt  writes:


 > Wow.  Nice work!

Thanks. I pushed things to Worg, if you haven't seen already 
(https://orgmode.org/worg/org-contrib/babel/languages/ob-doc-shell.html). 
If you read it and find anything missing or unclear, please let 
me know. I'm still waiting on work to sign the FSF disclaimer in 
order to work on ob-shell.el. I might just implement something 
on the side and use that to inform any future contributions.


I was looking at the Worg page when I wrote "Wow" :)

Many thanks for this contribution.

All the best,
Tom

--
Thomas S. Dye
https://tsdye.online/tsdye

Re: [PATCH] ob-shell-test, test-ob-shell and introduction

[Matt]

 > Wow.  Nice work!

Thanks. I pushed things to Worg, if you haven't seen already 
(https://orgmode.org/worg/org-contrib/babel/languages/ob-doc-shell.html). If 
you read it and find anything missing or unclear, please let me know. I'm still 
waiting on work to sign the FSF disclaimer in order to work on ob-shell.el. I 
might just implement something on the side and use that to inform any future 
contributions.

Re: [PATCH] ob-shell-test, test-ob-shell and introduction

[Thomas S. Dye]

Aloha Matt,

Wow.  Nice work!

All the best,
Tom

Matt  writes:

 > Contributions to Worg aren't similarly restricted.  Feel free 
 > to 
 > push material there in the meantime.


Looks like the email finally got sent to the right person at my 
company. Who knows how long it will take for them to get the FSF 
disclaimer back to me...


Now that I'm on holiday, I've got a little more time to 
contribute to Worg. Here's a few patches showing what I've come 
up with. I'll finish it up in the next few days, hopefully.  Not 
sure if the patches are formed correctly,  sorry about that if 
not. It's late here and I wanted to show progress. I'll worry 
about those kinds of details later. :)



--
Thomas S. Dye
https://tsdye.online/tsdye

Re: [PATCH] ob-shell-test, test-ob-shell and introduction

[Thomas S. Dye]

Aloha Matt,

Matt  writes:

 > Contributions to Worg aren't similarly restricted.  Feel free 
 > to 
 > push material there in the meantime.


Looks like the email finally got sent to the right person at my 
company. Who knows how long it will take for them to get the FSF 
disclaimer back to me...


Now that I'm on holiday, I've got a little more time to 
contribute to Worg. Here's a few patches showing what I've come 
up with. I'll finish it up in the next few days, hopefully.  Not 
sure if the patches are formed correctly,  sorry about that if 
not. It's late here and I wanted to show progress. I'll worry 
about those kinds of details later. :)


Good news.

FYI, you don't need to format patches for Worg.  IIRC, you've 
registered at sr.ht and sent your user name to Bastien, so you 
should be able to push directly to Worg when you're ready.


Thanks for your help with ob-doc-shell.org.

All the best,
Tom
--
Thomas S. Dye
https://tsdye.online/tsdye

Re: [PATCH] ob-shell-test, test-ob-shell and introduction

[Matt]

 > Contributions to Worg aren't similarly restricted.  Feel free to 
 > push material there in the meantime.

Looks like the email finally got sent to the right person at my company. Who 
knows how long it will take for them to get the FSF disclaimer back to me...

Now that I'm on holiday, I've got a little more time to contribute to Worg. 
Here's a few patches showing what I've come up with. I'll finish it up in the 
next few days, hopefully.  Not sure if the patches are formed correctly,  sorry 
about that if not. It's late here and I wanted to show progress. I'll worry 
about those kinds of details later. :)


0001-Draft-introduction-and-setup.patch
Description: Binary data


0001-org-contrib-babel-languages-ob-doc-shell.org-Create-.patch
Description: Binary data

Re: [PATCH] ob-shell-test, test-ob-shell and introduction

[Thomas S. Dye]

Aloha Matt,

Matt  writes:

 > > I just verified with my employer that my contract grants an 
 > > exception for this
 > > project. Just emailed the request to ass...@gnu.org. 

Not surprisingly the FSF hasn't resources to verify my 
contract's exception and needs a written disclaimer from my 
employer. I'm waiting on this now.


 > Feel free to develop and
 > share patches before the assignment is complete, we’ll just 
 > wait till it’s gone
 > through before merging 
 
Given the contract exception, I'll start moving forward again 
with the assumption that I'll eventually get that written 
disclaimer.


 > Hope that helps.
 
It does, thank you. It's nice to know that my words aren't going 
into the void. :)


Good news.

Contributions to Worg aren't similarly restricted.  Feel free to 
push material there in the meantime.


All the best,
Tom

--
Thomas S. Dye
https://tsdye.online/tsdye

Re: [PATCH] ob-shell-test, test-ob-shell and introduction

[Matt]

 > > I just verified with my employer that my contract grants an exception for 
 > > this
 > > project. Just emailed the request to ass...@gnu.org. 

Not surprisingly the FSF hasn't resources to verify my contract's exception and 
needs a written disclaimer from my employer. I'm waiting on this now.

 > Feel free to develop and
 > share patches before the assignment is complete, we’ll just wait till it’s 
 > gone
 > through before merging 
 
Given the contract exception, I'll start moving forward again with the 
assumption that I'll eventually get that written disclaimer.

 > Hope that helps.
 
It does, thank you. It's nice to know that my words aren't going into the void. 
:)

Re: [PATCH] ob-shell-test, test-ob-shell and introduction

[Timothy]

Hi Matt,

>  > [FSF copyright assignment]. Have you done that yet?
>
> I just verified with my employer that my contract grants an exception for this
> project. Just emailed the request to ass...@gnu.org. Also, got access from
> Bastien for worg. I figure it’s probably best to reserve any more changes ’til
> the paper work is done?

Great to hear that won’t get in the way of things . Feel free to develop and
share patches before the assignment is complete, we’ll just wait till it’s gone
through before merging (the process should just be a quick email or two and take
no more than a few days, just let us know if it’s dragging out).

Hope that helps.

All the best,
Timothy

Re: [PATCH] ob-shell-test, test-ob-shell and introduction

[Matt]

 > [FSF copyright assignment]. Have you done that yet?
 
I just verified with my employer that my contract grants an exception for this 
project.  Just emailed the request to ass...@gnu.org.  Also, got access from 
Bastien for worg. I figure it's probably best to reserve any more changes 'til 
the paper work is done?
 

Re: [PATCH] ob-shell-test, test-ob-shell and introduction

[Thomas S. Dye]

Aloha Matt,

Matt  writes:


Hi,

I'm interested in getting async into ob-shell.el. Since I've 
never contributed before, I figure it'd be good to start with a 
few easy tasks.


It looks to me like the stdin and cmdline header args aren't 
documented anywhere (at least I couldn't find anything). To make 
sure I'm using them correctly before making a patch for the 
manual, here are some tests.


Please let me know if things look okay. It wasn't clear to me 
how to send along a message with git send-email, so I formatted 
these patches and included them as an attachment. Is that fine?


Thanks,
Matt


Unfortunately, there is no ob-doc-shell.org at 
worg/org-contrib/babel/languages/.


There is an ob-doc-template.org in case you'd like to contribute 
ob-doc-shell.org :)


All the best,
Tom
--
Thomas S. Dye
https://tsdye.online/tsdye

Re: [PATCH] ob-shell-test, test-ob-shell and introduction

[Timothy]

Hi Matt,

> I’m interested in getting async into ob-shell.el. Since I’ve never contributed
>before, I figure it’d be good to start with a few easy tasks.

Fantastic! Great to hear from you, and I hope it goes well. Feel free to send
further emails for if you get stuck. If you intend to do more than just one or
two small tweaks (it sounds like you have bigger plans than that), you’ll need
what’s known as [FSF copyright assignment]. Have you done that yet?

> It looks to me like the stdin and cmdline header args aren’t documented 
> anywhere
> (at least I couldn’t find anything). To make sure I’m using them correctly
> before making a patch for the manual, here are some tests.

Great! I’m not overly familiar with these, so I’ll leave other people to take a
look at the tests you’ve written, but your approach sounds good. 

> Please let me know if things look okay. It wasn’t clear to me how to send 
> along
> a message with git send-email, so I formatted these patches and included them 
> as
> an attachment. Is that fine?

Patches attached to an email are perfectly fine here.
Thanks for getting in touch, and I hope to see you around .

All the best,
Timothy


[FSF copyright assignment]