search

astahlman / ob-async

Asynchronous src_block execution for org-babel
348 stars 32 forks source link

C-c C-c on name property executes block synchronously #10

Closed ag91 closed 7 years ago

ag91 commented 7 years ago

Hello! Thanks hugely for this extension of org mode! I can finally run the slow stuff while taking notes in org: that's wonderful!

There is a really minor issue, that can surprise someone else: here I report it. Say you have the following block and ob-async installed:

#+NAME: sleep 5 seconds
#+BEGIN_SRC sh :async
sleep 5s
#+END_SRC

when your point is on the NAME property and you try to run the block with C-c C-c, the synchronous version of the block is run. I am not sure if you intended this as a feature, but for me is hard to remember to not run the block on the name bit.

Thanks a lot again for your effort and for sharing this mode with us!

stnutt commented 7 years ago

I don't think this is an intended feature. This is because the async code only kicks in if (org-in-src-block-p) evaluates to t, which isn't true when the cursor is on #+NAME. This is essentially the same problem as #11 (since the cursor is on #+CALL in that one). This could be resolved by doing additional checks and looking for an associated src block, but it could also be resolved by advising all calls to org-babel-execute-src-block with the async version, instead of hooking into org-ctrl-c-ctrl-c-hook and doing these checks manually. That way the existing org code is in charge of determining whether any babel execution needs to take place, and the async execute function merely needs to determine if the executing block has an :async parameter (and call the regular execute function if there is no such parameter).

I have a fix for this and #11 and will try to submit a pull request. Although, the downside of the fix is the installation instructions change from adding to the hook, to instead simply loading the package to trigger the advice.

astahlman commented 7 years ago

Thanks for reporting this. @stnutt is right, it's not intentional.

I like the proposed fix. Regarding changing the installation instructions, I'm not too worried about it provided that we don't break anything for users who still have the org-ctrl-c-ctrl-c-hook installed. Maybe we could change the definition of the hooked function to simply echo a message warning the user that it is no longer needed.

ag91 commented 7 years ago

Whoa! Guys thanks! This is a cool and prompt solution :D I wish to contribute a little to it: @stnutt forgot a 'no' in the warning message :P I think it should be: (warn "ob-async-org-babel-execute-src-block is no longer needed in org-ctrl-c-ctrl-c-hook")

Thanks again!!

astahlman commented 7 years ago

Whoops, you're right, thanks for catching that. I've added the missing 'no'.

wmealing commented 7 years ago

oh damn, sorry i just lodged an issue on that ^ exact issue.. maybe my install is behind or something.