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
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
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
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
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
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
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
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
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
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
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 >
[PATCH] Re: New source block results option for attaching file to node
Updated patch; fixed typos and wrapped the commit message to 72 characters Thinking about the input files issue some more, I wonder if an attach keyword wouldn't better function as setting the working directory to the node's attachment directory and then fixing up any inserted link to use "attachment:" if it's relative to that. I've been working with this for a bit in my own workflow and it's been useful, but I'll see about maybe experimenting with some other approaches as well. Moving the referenced file to the attachment directory never felt completely right to me. On Wed, Sep 1, 2021 at 1:01 PM Ryan Scott wrote: > I hadn't thought about input directories much as my usage of > graphviz/gnuplots is through [essentially] DSLs that I made for them, so > the blocks are actually elisp. > Perhaps a convenient way of setting the working directory to the > attachment directory per-block makes sense? > > My own personal coding style doesn't include a hard line limit, so it just > wasn't on my mind. > > I'll get my patch updated and submitted shortly. Thanks for the help! > > On Wed, Sep 1, 2021 at 7:44 AM Ihor Radchenko wrote: > >> Ryan Scott writes: >> >> The patch looks fine for me except a typo: >> >> > + by the source block to the nodes attachmen directory (as >> ^attachment >> >> > org-attach-dir is a function for me (latest org pulled using >> straight.el) >> > org/lisp/org-attach.el:327. >> >> Timothy probably does not have (require 'org-attach) in his personal >> config. However, it should not be an issue for your patch with the >> autoload you added. >> >> > The primary use case is src blocks that generate files, in my case >> usually >> > gnuplot or graphviz, and return a file path. With a collection of org >> files >> > in a directory, organization can get messy, and creating an >> organizational >> > scheme essentially recreates the attachment directory design. >> >> I am also using attach directories for gnuplot output. Your approach is >> fine, but what about input directory? I find it a bit awkward to store >> input files alongside with the main .org file, while keeping the output >> images as attachments. >> >> I personally prefer to set the working dir for gnuplot like >> >> #+begin_src gnuplot :dir (org-attach-dir) >> >> With my approach, both the input and output files are going to be in the >> attach dir. >> >> I even go as far as making attach dir my default directory of all the >> code blocks. >> >> Though your patch may be useful when input directory is read-only or >> even remote. >> >> > Another approach would be to instead only modify org to have hooks (or >> any >> > other callback mechanism really) that are run on link insertion and have >> > access to the result-params for the block. The rest of this could then >> be a >> > separate package easily enough. Would that be a better approach as it >> would >> > allow the org core to not be so tightly coupled to org-attach? >> >> org-attach is in the Org core. It should not be a problem supporting >> org-attach in org babel. >> >> > I'm using magit; I just don't normally restrain myself to the line >> length. >> > I'll make sure to do that for submitted patches here. >> >> You may find flycheck-mode useful to hint things like line length. >> >> > In terms of this mailing list and overall contribution process, how >> best to >> > remedy things for the patch? Just modify it and reply with the modified >> > patch as an attachment? >> >> Yep. Just submit the updated patch. Preferably, also add [PATCH] at the >> beginning of the message topic. Then, the patch will also be shown in >> updates.orgmode.org >> >> Best, >> Ihor >> > 0001-ob-core-Added-option-for-attaching-file-from-babel-s.patch Description: Binary data
Re: New source block results option for attaching file to node
I hadn't thought about input directories much as my usage of graphviz/gnuplots is through [essentially] DSLs that I made for them, so the blocks are actually elisp. Perhaps a convenient way of setting the working directory to the attachment directory per-block makes sense? My own personal coding style doesn't include a hard line limit, so it just wasn't on my mind. I'll get my patch updated and submitted shortly. Thanks for the help! On Wed, Sep 1, 2021 at 7:44 AM Ihor Radchenko wrote: > Ryan Scott writes: > > The patch looks fine for me except a typo: > > > + by the source block to the nodes attachmen directory (as > ^attachment > > > org-attach-dir is a function for me (latest org pulled using straight.el) > > org/lisp/org-attach.el:327. > > Timothy probably does not have (require 'org-attach) in his personal > config. However, it should not be an issue for your patch with the > autoload you added. > > > The primary use case is src blocks that generate files, in my case > usually > > gnuplot or graphviz, and return a file path. With a collection of org > files > > in a directory, organization can get messy, and creating an > organizational > > scheme essentially recreates the attachment directory design. > > I am also using attach directories for gnuplot output. Your approach is > fine, but what about input directory? I find it a bit awkward to store > input files alongside with the main .org file, while keeping the output > images as attachments. > > I personally prefer to set the working dir for gnuplot like > > #+begin_src gnuplot :dir (org-attach-dir) > > With my approach, both the input and output files are going to be in the > attach dir. > > I even go as far as making attach dir my default directory of all the > code blocks. > > Though your patch may be useful when input directory is read-only or > even remote. > > > Another approach would be to instead only modify org to have hooks (or > any > > other callback mechanism really) that are run on link insertion and have > > access to the result-params for the block. The rest of this could then > be a > > separate package easily enough. Would that be a better approach as it > would > > allow the org core to not be so tightly coupled to org-attach? > > org-attach is in the Org core. It should not be a problem supporting > org-attach in org babel. > > > I'm using magit; I just don't normally restrain myself to the line > length. > > I'll make sure to do that for submitted patches here. > > You may find flycheck-mode useful to hint things like line length. > > > In terms of this mailing list and overall contribution process, how best > to > > remedy things for the patch? Just modify it and reply with the modified > > patch as an attachment? > > Yep. Just submit the updated patch. Preferably, also add [PATCH] at the > beginning of the message topic. Then, the patch will also be shown in > updates.orgmode.org > > Best, > Ihor >
Re: New source block results option for attaching file to node
Great. Yeah I tried to adhere to the commit format the best I could. org-attach-dir is a function for me (latest org pulled using straight.el) org/lisp/org-attach.el:327. The primary use case is src blocks that generate files, in my case usually gnuplot or graphviz, and return a file path. With a collection of org files in a directory, organization can get messy, and creating an organizational scheme essentially recreates the attachment directory design. Another approach would be to instead only modify org to have hooks (or any other callback mechanism really) that are run on link insertion and have access to the result-params for the block. The rest of this could then be a separate package easily enough. Would that be a better approach as it would allow the org core to not be so tightly coupled to org-attach? I'm using magit; I just don't normally restrain myself to the line length. I'll make sure to do that for submitted patches here. In terms of this mailing list and overall contribution process, how best to remedy things for the patch? Just modify it and reply with the modified patch as an attachment? On Tue, Aug 31, 2021 at 4:24 AM Timothy wrote: > Hi Ryan, > > Thanks for submitting your first patch to Org! > > Regarding the purpose of the patch, I take it this is useful with source > blocks that return a file patch and should be added as an attachment? Would > you mind expanding on this use case? Oh, and org-attach-dir doesn’t show > up a function or variable for me, I take it you meant something else? > > Lastly, with your commit message, you’ve clearly looked at the requested > commit message format . There’s the odd typo (“theh”) and the line length > is too long. It’s good if you hard line-wrap to 72 characters (if you use > Magit, this should happen automatically FYI). Other than that it looks > good to me. > > All the best, > *Timothy* > > * From*: Ryan Scott <%22ryan+scott%22+%3cr...@vicarious-living.com%3E> > * Subject*: New source block results option for attaching file to node > * To*: emacs-orgmode@gnu.org > <%22emacs-orgm...@gnu.org%22+%3cemacs-orgm...@gnu.org%3E> > * Date*: Thu, 26 Aug 2021 16:48:50 +0800 > An additional option for use with ":results file" that moves the returned > path to the node attachment directory (as returned by org-attach-dir), > creating it if necessary. > > First time submitting a patch. Any feedback is appreciated. > > -ryan > >
New source block results option for attaching file to node
An additional option for use with ":results file" that moves the returned path to the node attachment directory (as returned by org-attach-dir), creating it if necessary. First time submitting a patch. Any feedback is appreciated. -ryan 0001-ob-core-Added-option-for-attaching-file-from-babel-s.patch Description: Binary data
