Re: [PATCH] Re: New source block results option for attaching file to node
Awesome! Thanks again for all the help. It's been a crash course in org internals and the contribution process. Well prepared for next time to go much smoother. Cheers, -ryan On Tue, Jun 14, 2022 at 6:47 AM Ihor Radchenko wrote: > Ryan Scott writes: > > > I put together a clean setup in docker for running the tests and believe > > I've gotten to the source of the problem. > > However I've thought that a few times now. > > Would you be interested to share it? It might be helpful for other > people trying to develop on Windows. > > > This version of the patch has all tests passing. > > The problem was in org-results-to-file and the attach dir detection. It > > both had problems with an introduced assumption that the buffer-file-name > > would be non-nil (causing several tests to fail) as well as being > generally > > overzealous in detecting file result paths as attachment links. > > Thanks! > > Applied onto main via 226119124 with several amendments. > > I have added some missing "." at the end of sentences, made sure that > all sentences are separated by doube " " as required by our conventions > and dropped `quoting' from non-symbols. > > Best, > Ihor >
Re: [PATCH] Re: New source block results option for attaching file to node
Ryan Scott writes: > I put together a clean setup in docker for running the tests and believe > I've gotten to the source of the problem. > However I've thought that a few times now. Would you be interested to share it? It might be helpful for other people trying to develop on Windows. > This version of the patch has all tests passing. > The problem was in org-results-to-file and the attach dir detection. It > both had problems with an introduced assumption that the buffer-file-name > would be non-nil (causing several tests to fail) as well as being generally > overzealous in detecting file result paths as attachment links. Thanks! Applied onto main via 226119124 with several amendments. I have added some missing "." at the end of sentences, made sure that all sentences are separated by doube " " as required by our conventions and dropped `quoting' from non-symbols. Best, Ihor
Re: [PATCH] Re: New source block results option for attaching file to node
I put together a clean setup in docker for running the tests and believe
I've gotten to the source of the problem.
However I've thought that a few times now.
This version of the patch has all tests passing.
The problem was in org-results-to-file and the attach dir detection. It
both had problems with an introduced assumption that the buffer-file-name
would be non-nil (causing several tests to fail) as well as being generally
overzealous in detecting file result paths as attachment links.
On Mon, Jun 13, 2022 at 10:55 PM Ryan Scott
wrote:
> Strange. I'll figure out a better setup for running the tests and get to
> the bottom of that.
> Thanks for the help.
>
> On Tue, Jun 14, 2022, 00:10 Ihor Radchenko wrote:
>
>> Ryan Scott writes:
>>
>> > Ah sorry about that. I'm on a windows laptop and didn't have make, so
>> was
>> > testing interactively and they were passing.
>> > I cleaned that up and remove the f-* usage and verified under Ubuntu (on
>> > WSL) that the new tests are passing. I was getting some failures with
>> > unrelated tests, but also get those in master as well.
>>
>> I am still getting test failures using Emacs 28.
>> All the tests are passing on main.
>>
>> Example backtrace:
>>
>> file-name-directory(nil)
>> org-babel-result-to-file("/tmp/test.txt" nil attachment)
>> org-babel-insert-result("/tmp/test.txt" ("replace" "value" "file" "g
>> org-babel-execute-src-block()
>> (progn (org-mode) (let ((point (string-match "" inside-text))
>> (unwind-protect (progn (org-mode) (let ((point (string-match "> (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn
>> (let ((temp-buffer (generate-new-buffer " *temp*" t))) (save-current
>> (let ((inside-text (if (stringp "\n#+begin_src shell :results
>> (let ((lexical-binding nil)) (let ((inside-text (if (stringp "\n> (lambda nil (let ((lexical-binding nil)) (let ((inside-text (if (str
>> ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
>> ert-run-test(#s(ert-test :name test-ob/result-graphics-link-type-hea
>> ert-run-or-rerun-test(#s(ert--stats :selector "test-ob" :tests ... :
>> ert-run-tests("test-ob" #f(compiled-function (event-type event
>> ert-run-tests-batch("test-ob")
>> ert-run-tests-batch-and-exit("test-ob")
>> (let ((org-id-track-globally t) (org-test-selector (if org-test-sele
>> org-test-run-batch-tests("test-ob")
>> command-line-1(("--eval" "(setq vc-handled-backends nil org-startup-
>> command-line()
>> normal-top-level()
>> Test test-ob/result-graphics-link-type-header-argument condition:
>> (wrong-type-argument stringp nil)
>>FAILED 129/140 test-ob/result-graphics-link-type-header-argument
>> (0.006189 sec)
>>
>> Best,
>> Ihor
>>
>
org-src-block-results-attach-dir.patch
Description: Binary data
Re: [PATCH] Re: New source block results option for attaching file to node
Strange. I'll figure out a better setup for running the tests and get to
the bottom of that.
Thanks for the help.
On Tue, Jun 14, 2022, 00:10 Ihor Radchenko wrote:
> Ryan Scott writes:
>
> > Ah sorry about that. I'm on a windows laptop and didn't have make, so was
> > testing interactively and they were passing.
> > I cleaned that up and remove the f-* usage and verified under Ubuntu (on
> > WSL) that the new tests are passing. I was getting some failures with
> > unrelated tests, but also get those in master as well.
>
> I am still getting test failures using Emacs 28.
> All the tests are passing on main.
>
> Example backtrace:
>
> file-name-directory(nil)
> org-babel-result-to-file("/tmp/test.txt" nil attachment)
> org-babel-insert-result("/tmp/test.txt" ("replace" "value" "file" "g
> org-babel-execute-src-block()
> (progn (org-mode) (let ((point (string-match "" inside-text))
> (unwind-protect (progn (org-mode) (let ((point (string-match " (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn
> (let ((temp-buffer (generate-new-buffer " *temp*" t))) (save-current
> (let ((inside-text (if (stringp "\n#+begin_src shell :results
> (let ((lexical-binding nil)) (let ((inside-text (if (stringp "\n (lambda nil (let ((lexical-binding nil)) (let ((inside-text (if (str
> ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
> ert-run-test(#s(ert-test :name test-ob/result-graphics-link-type-hea
> ert-run-or-rerun-test(#s(ert--stats :selector "test-ob" :tests ... :
> ert-run-tests("test-ob" #f(compiled-function (event-type event
> ert-run-tests-batch("test-ob")
> ert-run-tests-batch-and-exit("test-ob")
> (let ((org-id-track-globally t) (org-test-selector (if org-test-sele
> org-test-run-batch-tests("test-ob")
> command-line-1(("--eval" "(setq vc-handled-backends nil org-startup-
> command-line()
> normal-top-level()
> Test test-ob/result-graphics-link-type-header-argument condition:
> (wrong-type-argument stringp nil)
>FAILED 129/140 test-ob/result-graphics-link-type-header-argument
> (0.006189 sec)
>
> Best,
> Ihor
>
Re: [PATCH] Re: New source block results option for attaching file to node
Ryan Scott writes:
> Ah sorry about that. I'm on a windows laptop and didn't have make, so was
> testing interactively and they were passing.
> I cleaned that up and remove the f-* usage and verified under Ubuntu (on
> WSL) that the new tests are passing. I was getting some failures with
> unrelated tests, but also get those in master as well.
I am still getting test failures using Emacs 28.
All the tests are passing on main.
Example backtrace:
file-name-directory(nil)
org-babel-result-to-file("/tmp/test.txt" nil attachment)
org-babel-insert-result("/tmp/test.txt" ("replace" "value" "file" "g
org-babel-execute-src-block()
(progn (org-mode) (let ((point (string-match "" inside-text))
(unwind-protect (progn (org-mode) (let ((point (string-match "#+begin_src shell :results
(let ((lexical-binding nil)) (let ((inside-text (if (stringp "\n
Re: [PATCH] Re: New source block results option for attaching file to node
Ah sorry about that. I'm on a windows laptop and didn't have make, so was testing interactively and they were passing. I cleaned that up and remove the f-* usage and verified under Ubuntu (on WSL) that the new tests are passing. I was getting some failures with unrelated tests, but also get those in master as well. Unless I'm missing something it looks like test-ob-core/dir-mkdirp covers most other :dir usage. I've remove the ID creation altogether and throw an error if org-attach-dir is nil at that point. The error directs the user to add :ID: or :DIR: and we don't need to do any guessing on their behalf. Implicit ID creation should likely live elsewhere anyway. On Sat, Jun 11, 2022 at 5:49 AM Ihor Radchenko wrote: > Ryan Scott writes: > > > Had no experience with the :DIR: property or writing unit tests for Org, > > but I think I've got both covered now. > > The ID creation prompting now only happens if there is no result from > > org-attach-dir, which should address the :DIR: case. > > Thanks! > > > Let me know if there's anything about those tests that I should modify. > > Yeah. They do not pass, currently... > You can try yourself by running make test from Org git directory. > > >> > + ((or '(:dir . attach) '(:dir . "'attach")) > >> > + (unless (org-id-get) > >> > + (if (or noninteractive (y-or-n-p (format "Create ID for > >> entry \"%s\"?" > >> > + > (org-get-heading > >> t t t t > >> > + (org-id-get-create) > >> > + (error "Can't attach to entry \"%s\". Entry has no ID" > >> > + (org-get-heading t t t t > > Unconditional ID creation for noninteractive is a bad idea. It is safer > to throw an error. > > This code also generates warnings: > > > In end of data: > ob-core.el:2788:21: Warning: the function ‘org-id-get-create’ is not known > to > be defined. > ob-core.el:2787:58: Warning: the function ‘org-get-heading’ is not known > to be > defined. > ob-core.el:2785:23: Warning: the function ‘org-id-get’ is not known to be > defined. > ob-core.el:2792:38: Warning: the function ‘org-attach-dir’ is not known to > be > defined. > > Note that not all the ideas use org-id. Hence, `org-id-get-create' may > not be available during runtime. Adding (require 'org-id) is not a good > idea either (there will be other side-effects). So, if org-id is not > loaded, it will be better to just throw an error. > > > +(ert-deftest test-ob-core/dir-attach () > > + "Test :dir header using special 'attach value" > > + (should > > + (org-test-with-temp-text-in-file > > +"* Symbol > > +#+begin_src elisp :dir 'attach :results file > > +(f-write-text \"attachment testing\" 'utf-8 \"test.txt\") > > f-write-text requires f.el, which is external packages. You cannot use > it. > > Also, while you are here, you can as well add tests for other possible > :dir settings. > > Best, > Ihor > org-src-block-results-attach-dir.patch Description: Binary data
Re: [PATCH] Re: New source block results option for attaching file to node
Ryan Scott writes: >> > + (if (or noninteractive (y-or-n-p (format "Create ID for >> entry \"%s\"?" One more thing I forgot. Please, use `yes-or-no-p'. `y-or-n-p' is not a good idea - it is too easy to accidently confirm by hitting space. Best, Ihor
Re: [PATCH] Re: New source block results option for attaching file to node
Ryan Scott writes: > Had no experience with the :DIR: property or writing unit tests for Org, > but I think I've got both covered now. > The ID creation prompting now only happens if there is no result from > org-attach-dir, which should address the :DIR: case. Thanks! > Let me know if there's anything about those tests that I should modify. Yeah. They do not pass, currently... You can try yourself by running make test from Org git directory. >> > + ((or '(:dir . attach) '(:dir . "'attach")) >> > + (unless (org-id-get) >> > + (if (or noninteractive (y-or-n-p (format "Create ID for >> entry \"%s\"?" >> > + (org-get-heading >> t t t t >> > + (org-id-get-create) >> > + (error "Can't attach to entry \"%s\". Entry has no ID" >> > + (org-get-heading t t t t Unconditional ID creation for noninteractive is a bad idea. It is safer to throw an error. This code also generates warnings: In end of data: ob-core.el:2788:21: Warning: the function ‘org-id-get-create’ is not known to be defined. ob-core.el:2787:58: Warning: the function ‘org-get-heading’ is not known to be defined. ob-core.el:2785:23: Warning: the function ‘org-id-get’ is not known to be defined. ob-core.el:2792:38: Warning: the function ‘org-attach-dir’ is not known to be defined. Note that not all the ideas use org-id. Hence, `org-id-get-create' may not be available during runtime. Adding (require 'org-id) is not a good idea either (there will be other side-effects). So, if org-id is not loaded, it will be better to just throw an error. > +(ert-deftest test-ob-core/dir-attach () > + "Test :dir header using special 'attach value" > + (should > + (org-test-with-temp-text-in-file > +"* Symbol > +#+begin_src elisp :dir 'attach :results file > +(f-write-text \"attachment testing\" 'utf-8 \"test.txt\") f-write-text requires f.el, which is external packages. You cannot use it. Also, while you are here, you can as well add tests for other possible :dir settings. Best, Ihor
Re: [PATCH] Re: New source block results option for attaching file to node
Had no experience with the :DIR: property or writing unit tests for Org, but I think I've got both covered now. The ID creation prompting now only happens if there is no result from org-attach-dir, which should address the :DIR: case. Let me know if there's anything about those tests that I should modify. Thanks, -ryan On Fri, Jun 10, 2022 at 9:31 PM Ihor Radchenko wrote: > Ryan Scott writes: > > > I believe I have addressed your feedback, Ihor. > > Attached is the latest version of the patch. > > > >- Merged latest master > >- :post is now handled correctly (verified with example of :post usage > >in the example at > https://orgmode.org/manual/Results-of-Evaluation.html) > >- Added "(with quotes)" to help make the NEWS entry clearer > >- Changed the attach directory detection to use a string prefix check > > > > Let me know what you think. > > Thanks for the updated patch! > > > + ((or '(:dir . attach) '(:dir . "'attach")) > > + (unless (org-id-get) > > + (if (or noninteractive (y-or-n-p (format "Create ID for > entry \"%s\"?" > > + (org-get-heading > t t t t > > + (org-id-get-create) > > + (error "Can't attach to entry \"%s\". Entry has no ID" > > + (org-get-heading t t t t > > + (setq params (append > > + `((:dir . ,(org-attach-dir nil t)) > > + (:mkdirp . "yes")) > > + (assq-delete-all :dir (assq-delete-all :mkdir > params) > > Note that entry does not need :ID: property to have an attachment dir. > There is also :DIR: property. > > Also, it would be useful to add a test. See test-ob-core/dir-mkdirp in > testing/lisp/test-ob.el > > Best, > Ihor > org-src-block-results-attach-dir.patch Description: Binary data
Re: [PATCH] Re: New source block results option for attaching file to node
Ryan Scott writes: > I believe I have addressed your feedback, Ihor. > Attached is the latest version of the patch. > >- Merged latest master >- :post is now handled correctly (verified with example of :post usage >in the example at https://orgmode.org/manual/Results-of-Evaluation.html) >- Added "(with quotes)" to help make the NEWS entry clearer >- Changed the attach directory detection to use a string prefix check > > Let me know what you think. Thanks for the updated patch! > + ((or '(:dir . attach) '(:dir . "'attach")) > + (unless (org-id-get) > + (if (or noninteractive (y-or-n-p (format "Create ID for entry > \"%s\"?" > + (org-get-heading t t t > t > + (org-id-get-create) > + (error "Can't attach to entry \"%s\". Entry has no ID" > + (org-get-heading t t t t > + (setq params (append > + `((:dir . ,(org-attach-dir nil t)) > + (:mkdirp . "yes")) > + (assq-delete-all :dir (assq-delete-all :mkdir > params) Note that entry does not need :ID: property to have an attachment dir. There is also :DIR: property. Also, it would be useful to add a test. See test-ob-core/dir-mkdirp in testing/lisp/test-ob.el Best, Ihor
Re: [PATCH] Re: New source block results option for attaching file to node
I believe I have addressed your feedback, Ihor. Attached is the latest version of the patch. - Merged latest master - :post is now handled correctly (verified with example of :post usage in the example at https://orgmode.org/manual/Results-of-Evaluation.html) - Added "(with quotes)" to help make the NEWS entry clearer - Changed the attach directory detection to use a string prefix check Let me know what you think. On Thu, Apr 21, 2022 at 11:19 PM Ryan Scott wrote: > Great. Just making sure that this particular approach to this feature or > type of feature wasn't fundamentally flawed given the established behavior > of org. > Thanks. > > On Thu, Apr 21, 2022 at 11:02 PM Ihor Radchenko > wrote: > >> Ryan Scott writes: >> >> > With all of the layers and assumptions/behaviors inherent in the way src >> > block parameters are handled, do you feel like this is an approach that >> can >> > work, or should it be abandoned? >> >> I am not sure if I understand your concern. >> Your code is reusing the existing convention that current directory is >> managed via default-directory by ob-core.el. I do not see any problem >> here. >> >> Best, >> Ihor >> > org-src-block-results-attach-dir.patch Description: Binary data
Re: [PATCH] Re: New source block results option for attaching file to node
Great. Just making sure that this particular approach to this feature or type of feature wasn't fundamentally flawed given the established behavior of org. Thanks. On Thu, Apr 21, 2022 at 11:02 PM Ihor Radchenko wrote: > Ryan Scott writes: > > > With all of the layers and assumptions/behaviors inherent in the way src > > block parameters are handled, do you feel like this is an approach that > can > > work, or should it be abandoned? > > I am not sure if I understand your concern. > Your code is reusing the existing convention that current directory is > managed via default-directory by ob-core.el. I do not see any problem > here. > > Best, > Ihor >
Re: [PATCH] Re: New source block results option for attaching file to node
Ryan Scott writes: > With all of the layers and assumptions/behaviors inherent in the way src > block parameters are handled, do you feel like this is an approach that can > work, or should it be abandoned? I am not sure if I understand your concern. Your code is reusing the existing convention that current directory is managed via default-directory by ob-core.el. I do not see any problem here. Best, Ihor
Re: [PATCH] Re: New source block results option for attaching file to node
Yeah I just recently updated my fourth and realized there were some necessary conflicts to resolve with the latest. I'll take a look at addressing these issues and submit an updated patch. Surprised I missed a calling location and secretly hope that it was added recently for the sake of my poor ego. :) With all of the layers and assumptions/behaviors inherent in the way src block parameters are handled, do you feel like this is an approach that can work, or should it be abandoned? Thanks for looking into this. On Thu, Apr 21, 2022, 05:46 Ihor Radchenko wrote: > Ryan Scott writes: > > > Here's my latest patch. > > Uses special :dir value 'attach to use attachment directory as working > dir. > > Now prompts to create IDs for nodes that are missing. > > Solved a handful of issues with my previous versions of this and I've > been > > using it regularly for a bit now. > > > > I've added documentation and completed the copyright assignment to the > FSF. > > I have consulted Bastien about our merge policy and apparently it is ok > to merge staff into files without maintainer. ob-core.el is one of such > files. > > So, I did a more elaborate testing of your patch. I have some comments. > > Firstly, the patch does not apply onto current main. > > > +Setting =dir= to the symbol ~attach~ or the string ~"'attach"~ will > > ... > > +Passing the symbol ~attach~ or string ='attach= to the =:dir= option > > When I was trying to use your patch, I felt slightly confused about the > "'attach" part. It _looks_ like a type and I first tried to do :dir > "attach" yielding predictable lack of attachment: file link. > > Maybe you can say "or the string ~"'attach"~ (with quote)". > > > + (org-babel-result-to-file > > + result > > + (org-babel--file-desc (nth 2 info) result) > > + 'attachment > > There are only 2 calls to org-babel-result-to-file in the whole Org > codebase. And you did not change the second call, which yield strange > side-effects: > > #+NAME: attr_wrap > #+BEGIN_SRC sh :var data="" :var width="\\textwidth" :results output > echo "#+ATTR_LATEX: :width $width" > echo "$data" > #+END_SRC > > #+begin_src gnuplot :dir 'attach :file test.png > plot x > #+end_src > > #+RESULTS: > [[attachment:test.png]] > > #+begin_src gnuplot :dir 'attach :file test.png :post > attr_wrap(width="5cm", data=*this*) :results drawer > plot x > #+end_src > > #+RESULTS: > :results: > #+ATTR_LATEX: :width 5cm > [[file:data/4d/6a76f8-4016-4edf-9d26-e0b3a634dbc1/test20.png]] this is not expected > :end: > > > +If the optional TYPE is passed as 'attachment` and the path is a > > +descendant of the DEFAULT-DIRECTORY, the generated link will be > > +specified as an an \"attachment:\" style link" > > + (in-attach-dir (when (and request-attachment (> (length > result-file-name) attach-dir-len)) > > +(string= > > + (substring result-file-name 0 > attach-dir-len) > > + attach-dir > > This is a risky heuristics. > One can do something like > (setq org-attach-id-dir > "/tmp/alsjkdsalkjdlaskdjklasjdlkasjdlkasjdlkajdklasjdlasjlasdjk/") > and often get your heuristics fail. > Of course, it will require some terribly noncomplying ob-* library that > will create file disregarding default-directory, but still... > > > + (if (y-or-n-p (format "Create ID for entry \"%s\"?" > > + (org-get-heading t t t t))) > > This is a nice dialogue, but note that Emacs can run noninteractively or > execute source block during export (including asynchronous). I would > guard the query after consulting `noninteractive' variable. > > > Best, > Ihor >
Re: [PATCH] Re: New source block results option for attaching file to node
Ryan Scott writes: > Here's my latest patch. > Uses special :dir value 'attach to use attachment directory as working dir. > Now prompts to create IDs for nodes that are missing. > Solved a handful of issues with my previous versions of this and I've been > using it regularly for a bit now. > > I've added documentation and completed the copyright assignment to the FSF. I have consulted Bastien about our merge policy and apparently it is ok to merge staff into files without maintainer. ob-core.el is one of such files. So, I did a more elaborate testing of your patch. I have some comments. Firstly, the patch does not apply onto current main. > +Setting =dir= to the symbol ~attach~ or the string ~"'attach"~ will > ... > +Passing the symbol ~attach~ or string ='attach= to the =:dir= option When I was trying to use your patch, I felt slightly confused about the "'attach" part. It _looks_ like a type and I first tried to do :dir "attach" yielding predictable lack of attachment: file link. Maybe you can say "or the string ~"'attach"~ (with quote)". > + (org-babel-result-to-file > + result > + (org-babel--file-desc (nth 2 info) result) > + 'attachment There are only 2 calls to org-babel-result-to-file in the whole Org codebase. And you did not change the second call, which yield strange side-effects: #+NAME: attr_wrap #+BEGIN_SRC sh :var data="" :var width="\\textwidth" :results output echo "#+ATTR_LATEX: :width $width" echo "$data" #+END_SRC #+begin_src gnuplot :dir 'attach :file test.png plot x #+end_src #+RESULTS: [[attachment:test.png]] #+begin_src gnuplot :dir 'attach :file test.png :post attr_wrap(width="5cm", data=*this*) :results drawer plot x #+end_src #+RESULTS: :results: #+ATTR_LATEX: :width 5cm [[file:data/4d/6a76f8-4016-4edf-9d26-e0b3a634dbc1/test20.png]] +If the optional TYPE is passed as 'attachment` and the path is a > +descendant of the DEFAULT-DIRECTORY, the generated link will be > +specified as an an \"attachment:\" style link" > + (in-attach-dir (when (and request-attachment (> (length > result-file-name) attach-dir-len)) > +(string= > + (substring result-file-name 0 attach-dir-len) > + attach-dir This is a risky heuristics. One can do something like (setq org-attach-id-dir "/tmp/alsjkdsalkjdlaskdjklasjdlkasjdlkasjdlkajdklasjdlasjlasdjk/") and often get your heuristics fail. Of course, it will require some terribly noncomplying ob-* library that will create file disregarding default-directory, but still... > + (if (y-or-n-p (format "Create ID for entry \"%s\"?" > + (org-get-heading t t t t))) This is a nice dialogue, but note that Emacs can run noninteractively or execute source block during export (including asynchronous). I would guard the query after consulting `noninteractive' variable. Best, Ihor
Re: [PATCH] Re: New source block results option for attaching file to node
Here's my latest patch. Uses special :dir value 'attach to use attachment directory as working dir. Now prompts to create IDs for nodes that are missing. Solved a handful of issues with my previous versions of this and I've been using it regularly for a bit now. I've added documentation and completed the copyright assignment to the FSF. On Mon, Oct 4, 2021 at 6:05 PM Ryan Scott wrote: > I've been working through a few different approaches. What's shaping up is > something more general, having a special value for directory parameters > (i.e. 'attach) and auto-detection of link paths that are in the attachment > directory. > The latest iterations don't move any files around, so can't actually > enforce the output directory. That makes it safer overall as with my > initial patch if you were to return a path to something you *didn't* want > moved to your attachment directory you might get very surprising results. > > I'll post a new patch with a different approach in a little bit. > > On Mon, Oct 4, 2021 at 5:06 PM Christopher M. Miles > wrote: > >> Ihor Radchenko writes: >> >> Greg Minshall writes: >>> >>> i can imagine wanting to have input files and output files in separate directories. (for ease in "make clean", if for no other conceptual reason.) (but, probably i don't understand.) >>> I agree with this thought. We should separate two directories. >> >> Makes sense. Currently, there is :dir header arg to set working directory >>> (aka input files directory). Maybe we can introduce something like >>> :results-dir header arg to set the output directory? It's value can be a >>> directory path or symbol 'attach. >>> >>> `:results file :results-dir 'attach` will be equivalent of `:results >>> file attach` in the patch proposed by Ryan Scott. >>> >>> WDYT? >>> >> I agree with this idea. Use :results-dir 'attach is better. >> >> Will the patch be updated? >> >> Best, Ihor >>> >> >> <#secure method=pgpmime mode=sign> >> -- >> [ stardiviner ] >>I try to make every word tell the meaning that I want to express. >> >>Blog: https://stardiviner.github.io/ >>IRC(freenode): stardiviner, Matrix: stardiviner >>GPG: F09F650D7D674819892591401B5DF1C95AE89AC3 >> > org-src-block-results-attach-dir.patch Description: Binary data
Re: [PATCH] Re: New source block results option for attaching file to node
Ryan Scott writes: > I've been working through a few different approaches. What's shaping up is > something more general, having a special value > for directory parameters (i.e. 'attach) and auto-detection of link paths that > are in the attachment directory. > The latest iterations don't move any files around, so can't actually enforce > the output directory. That makes it safer > overall as with my initial patch if you were to return a path to something > you didn't want moved to your attachment > directory you might get very surprising results. I used to created a similar function extension called "ob-clojure-literate.el" for ob-clojure.el. One functionality is to save plotting result image to a header argument :dir specified directory. But it's a little complicated. Then later I found another solution through *header argument evaluation*: #+begin_src clojure :results file link :dir "data/images" :file "ob-clojure-incanter-move.png" :var dir=(concat (file-name-directory (buffer-file-name)) "data/images/") (use '(incanter core io charts stats)) (import '(java.io File)) (def hist (histogram (sample-normal 1000))) (save hist "ob-clojure-incanter-move.png") (.renameTo (File. "ob-clojure-incanter-move.png") (File. (str dir "ob-clojure-incanter-move.png"))) #+end_src But this code has a disadvantage: This solution breaks Literate Programming tangling concept by introduced un-reimplemented variable ~dir~. > > I'll post a new patch with a different approach in a little bit. > > On Mon, Oct 4, 2021 at 5:06 PM Christopher M. Miles > wrote: > > Ihor Radchenko writes: > > Greg Minshall writes: > > i can imagine wanting to have input files and output files in separate > directories. (for ease in "make > clean", if for no other conceptual reason.) (but, probably i don't > understand.) > > I agree with this thought. We should separate two directories. > > Makes sense. Currently, there is :dir header arg to set working directory > (aka input files directory). Maybe we > can introduce something like :results-dir header arg to set the output > directory? It's value can be a directory > path or symbol 'attach. > > `:results file :results-dir 'attach` will be equivalent of `:results file > attach` in the patch proposed by Ryan > Scott. > > WDYT? > > I agree with this idea. Use :results-dir 'attach is better. > > Will the patch be updated? > > Best, Ihor > > <#secure method=pgpmime mode=sign> > -- > [ stardiviner ] > I try to make every word tell the meaning that I want to express. > > Blog: https://stardiviner.github.io/ > IRC(freenode): stardiviner, Matrix: stardiviner > GPG: F09F650D7D674819892591401B5DF1C95AE89AC3 <#secure method=pgpmime mode=sign> -- [ stardiviner ] I try to make every word tell the meaning that I want to express. Blog: https://stardiviner.github.io/ IRC(freenode): stardiviner, Matrix: stardiviner GPG: F09F650D7D674819892591401B5DF1C95AE89AC3
Re: [PATCH] Re: New source block results option for attaching file to node
I've been working through a few different approaches. What's shaping up is something more general, having a special value for directory parameters (i.e. 'attach) and auto-detection of link paths that are in the attachment directory. The latest iterations don't move any files around, so can't actually enforce the output directory. That makes it safer overall as with my initial patch if you were to return a path to something you *didn't* want moved to your attachment directory you might get very surprising results. I'll post a new patch with a different approach in a little bit. On Mon, Oct 4, 2021 at 5:06 PM Christopher M. Miles wrote: > Ihor Radchenko writes: > > Greg Minshall writes: >> >> i can imagine wanting to have input files and output files in separate >>> directories. (for ease in "make clean", if for no other conceptual reason.) >>> (but, probably i don't understand.) >>> >> I agree with this thought. We should separate two directories. > > Makes sense. Currently, there is :dir header arg to set working directory >> (aka input files directory). Maybe we can introduce something like >> :results-dir header arg to set the output directory? It's value can be a >> directory path or symbol 'attach. >> >> `:results file :results-dir 'attach` will be equivalent of `:results file >> attach` in the patch proposed by Ryan Scott. >> >> WDYT? >> > I agree with this idea. Use :results-dir 'attach is better. > > Will the patch be updated? > > Best, Ihor >> > > <#secure method=pgpmime mode=sign> > -- > [ stardiviner ] >I try to make every word tell the meaning that I want to express. > >Blog: https://stardiviner.github.io/ >IRC(freenode): stardiviner, Matrix: stardiviner >GPG: F09F650D7D674819892591401B5DF1C95AE89AC3 >
Re: [PATCH] Re: New source block results option for attaching file to node
Ihor Radchenko writes: > Greg Minshall writes: > >> i can imagine wanting to have input files and >> output files in separate directories. (for ease in "make clean", if for >> no other conceptual reason.) (but, probably i don't understand.) I agree with this thought. We should separate two directories. > > Makes sense. Currently, there is :dir header arg to set working > directory (aka input files directory). Maybe we can introduce something > like :results-dir header arg to set the output directory? It's value can > be a directory path or symbol 'attach. > > `:results file :results-dir 'attach` will be equivalent of > `:results file attach` in the patch proposed by Ryan Scott. > > WDYT? I agree with this idea. Use ~:results-dir 'attach~ is better. Will the patch be updated? > > Best, > Ihor <#secure method=pgpmime mode=sign> -- [ stardiviner ] I try to make every word tell the meaning that I want to express. Blog: https://stardiviner.github.io/ IRC(freenode): stardiviner, Matrix: stardiviner GPG: F09F650D7D674819892591401B5DF1C95AE89AC3
Re: [PATCH] Re: New source block results option for attaching file to node
Would it be better then as a new option entirely that sets the default directory to the attachment directory and results in attachment links for any inserted paths that are under that? The attachment link detection could possibly be default behavior for link insertion, but i can imagine that might have broader implications. I also found and have a fix for my patch where the 'attach symbol gets converted to a string when using #+call to call a block that is defined with this option, which felt a little awkward in the code. I'll try this as a standalone option and see how that feels. That would carve out space for other options in handling attachments. On Sat, Oct 2, 2021, 01:31 Ihor Radchenko wrote: > Ryan Scott writes: > > >(default-directory > > -(or (and dir (file-name-as-directory dir)) default-directory)) > > +(or (and dir (if (eq dir 'attach) > > +(org-attach-dir t) > > + (file-name-as-directory dir))) > > + default-directory)) > > This does not always work. Some ob-*.el code (namely, ob-lisp have the > following: > > (let ((dir (if (assq :dir params) >(cdr (assq :dir params)) > default-directory))) > > As you can see, :dir parameter is overriding default-directory. If :dir > is set to symbol 'attach, execution will fail. > > I think that you also need to override :dir in the parameter list and > put actual path to attachment dir instead of symbol. That way, we will > not need to change every possible ob-*.el implementation to account for > new 'attach option. > > Also, marking this as patch. > > Best, > Ihor >
Re: [PATCH] Re: New source block results option for attaching file to node
Ryan Scott writes: >(default-directory > -(or (and dir (file-name-as-directory dir)) default-directory)) > +(or (and dir (if (eq dir 'attach) > +(org-attach-dir t) > + (file-name-as-directory dir))) > + default-directory)) This does not always work. Some ob-*.el code (namely, ob-lisp have the following: (let ((dir (if (assq :dir params) (cdr (assq :dir params)) default-directory))) As you can see, :dir parameter is overriding default-directory. If :dir is set to symbol 'attach, execution will fail. I think that you also need to override :dir in the parameter list and put actual path to attachment dir instead of symbol. That way, we will not need to change every possible ob-*.el implementation to account for new 'attach option. Also, marking this as patch. Best, Ihor
Re: [PATCH] Re: New source block results option for attaching file to node
Hi Ryan, I’ve just had a glance, but this looks much better to me than what was proposed earlier . Hopefully we’ll be able to get some feedback on this from others, and then see it merged . All the best, Timothy
Re: [PATCH] Re: New source block results option for attaching file to node
Okay, Had some time to put into this. Much happier with this approach as it doesn't require any file moving and generally leaves src blocks to their own devices. The short version is that specifying ":dir 'attach" for a block uses the directory from (org-attach-dir) as its working directory and any generated path that is a descendant of that directory will be converted to an "attachment:" link. ob-core.el/babel: Special handling for attachment links in src blocks * ob-core.el (org-babel-execute-src-block): Specifying the symbol 'attach` as the value of the `:dir' header now functions as ":dir (org-attach-dir)" (org-babel-result-to-file): Optional `TYPE' argument accepts symbol 'attachment` to fixup up paths under `DEFAULT-DIRECTORY' and use the link type "attachment:" when that is detected. (org-babel-insert-result): Pass symbol `attachment' as `TYPE' to `org-babel-result-to-file' when header `:dir' is set to symbol `attach' (org-babel-load-in-session, org-babel-initiate-session) ":dir 'attach" sets `default-directory' with "(org-attach-dir t)" * org-attach.el (org-attach-dir): Added autoload header to simplify dependencies necessary to support this feature (called in `org-babel-execute-src-block'). On Sun, Sep 5, 2021 at 6:56 AM Ryan Scott wrote: > Yeah your second example is what I'm thinking. It makes this all a fairly > concise extension of that existing mechanism and does away with the file > move after execution. > > On Sun, Sep 5, 2021, 06:21 Ihor Radchenko wrote: > >> Ryan Scott writes: >> >> > It might make sense to fix up inserted "file:" links that are under the >> > attachment directory to be "attachment:" style links by default anyway, >> no? >> > Then just being able to set the working directory to the attachment >> > directory easily would get the rest of the way there. >> >> I am not sure. If the user explicitly states that :dir is the attachment >> dir, it would make sense. However, what if the :dir is set explicitly >> like below? >> >> * Headline >> :PROPERTIES: >> :DIR: /actual/literal/path/to/attachment/dir >> :END: >> >> #+begin_src emacs-lisp :dir /actual/literal/path/to/attachment/dir >> ... >> >> #+RESULTS: >> attachment:... >> >> The results will be indeed inside the attachment directory. However, the >> :DIR: property may be changed at some point and the existing attachment: >> link will not point to real file. >> >> > So I suppose that would then mean having the :dir header accept the >> symbol >> > `attach' or something like that? >> > I'll play around and see what that looks like. >> >> The above example should lead to more expected behaviour if the user >> explicitly states that :dir is the attachment dir (even if it is going >> to be changed in future): >> >> * Headline >> :PROPERTIES: >> :DIR: /actual/literal/path/to/attachment/dir >> :END: >> >> #+begin_src emacs-lisp :dir 'attach >> ... >> >> #+RESULTS: >> attachment:... >> >> Best, >> Ihor >> > 0001-ob-core.el-babel-Special-handling-for-attachment-lin.patch Description: Binary data
Re: [PATCH] Re: New source block results option for attaching file to node
Yeah your second example is what I'm thinking. It makes this all a fairly concise extension of that existing mechanism and does away with the file move after execution. On Sun, Sep 5, 2021, 06:21 Ihor Radchenko wrote: > Ryan Scott writes: > > > It might make sense to fix up inserted "file:" links that are under the > > attachment directory to be "attachment:" style links by default anyway, > no? > > Then just being able to set the working directory to the attachment > > directory easily would get the rest of the way there. > > I am not sure. If the user explicitly states that :dir is the attachment > dir, it would make sense. However, what if the :dir is set explicitly > like below? > > * Headline > :PROPERTIES: > :DIR: /actual/literal/path/to/attachment/dir > :END: > > #+begin_src emacs-lisp :dir /actual/literal/path/to/attachment/dir > ... > > #+RESULTS: > attachment:... > > The results will be indeed inside the attachment directory. However, the > :DIR: property may be changed at some point and the existing attachment: > link will not point to real file. > > > So I suppose that would then mean having the :dir header accept the > symbol > > `attach' or something like that? > > I'll play around and see what that looks like. > > The above example should lead to more expected behaviour if the user > explicitly states that :dir is the attachment dir (even if it is going > to be changed in future): > > * Headline > :PROPERTIES: > :DIR: /actual/literal/path/to/attachment/dir > :END: > > #+begin_src emacs-lisp :dir 'attach > ... > > #+RESULTS: > attachment:... > > Best, > Ihor >
Re: [PATCH] Re: New source block results option for attaching file to node
Ryan Scott writes: > It might make sense to fix up inserted "file:" links that are under the > attachment directory to be "attachment:" style links by default anyway, no? > Then just being able to set the working directory to the attachment > directory easily would get the rest of the way there. I am not sure. If the user explicitly states that :dir is the attachment dir, it would make sense. However, what if the :dir is set explicitly like below? * Headline :PROPERTIES: :DIR: /actual/literal/path/to/attachment/dir :END: #+begin_src emacs-lisp :dir /actual/literal/path/to/attachment/dir ... #+RESULTS: attachment:... The results will be indeed inside the attachment directory. However, the :DIR: property may be changed at some point and the existing attachment: link will not point to real file. > So I suppose that would then mean having the :dir header accept the symbol > `attach' or something like that? > I'll play around and see what that looks like. The above example should lead to more expected behaviour if the user explicitly states that :dir is the attachment dir (even if it is going to be changed in future): * Headline :PROPERTIES: :DIR: /actual/literal/path/to/attachment/dir :END: #+begin_src emacs-lisp :dir 'attach ... #+RESULTS: attachment:... Best, Ihor
Re: [PATCH] Re: New source block results option for attaching file to node
That's starting to sound pretty good. It might make sense to fix up inserted "file:" links that are under the attachment directory to be "attachment:" style links by default anyway, no? Then just being able to set the working directory to the attachment directory easily would get the rest of the way there. So I suppose that would then mean having the :dir header accept the symbol `attach' or something like that? I'll play around and see what that looks like. On Thu, Sep 2, 2021 at 8:09 PM Ihor Radchenko wrote: > Greg Minshall writes: > > > i can imagine wanting to have input files and > > output files in separate directories. (for ease in "make clean", if for > > no other conceptual reason.) (but, probably i don't understand.) > > Makes sense. Currently, there is :dir header arg to set working > directory (aka input files directory). Maybe we can introduce something > like :results-dir header arg to set the output directory? It's value can > be a directory path or symbol 'attach. > > `:results file :results-dir 'attach` will be equivalent of > `:results file attach` in the patch proposed by Ryan Scott. > > WDYT? > > Best, > Ihor >
Re: [PATCH] Re: New source block results option for attaching file to node
Greg Minshall writes: > i can imagine wanting to have input files and > output files in separate directories. (for ease in "make clean", if for > no other conceptual reason.) (but, probably i don't understand.) Makes sense. Currently, there is :dir header arg to set working directory (aka input files directory). Maybe we can introduce something like :results-dir header arg to set the output directory? It's value can be a directory path or symbol 'attach. `:results file :results-dir 'attach` will be equivalent of `:results file attach` in the patch proposed by Ryan Scott. WDYT? Best, Ihor
Re: [PATCH] Re: New source block results option for attaching file to node
Ryan, et al., i'm not entirely following the discussion, as i don't use "attaching". but, fwiw, if i did, i can imagine wanting to have input files and output files in separate directories. (for ease in "make clean", if for no other conceptual reason.) (but, probably i don't understand.) cheers, Greg
