Re: [ANN] lisp/ob-tangle-sync.el

[Mehmet Tekman <mtekman89@gmail.com>]

Ihor Radchenko <yantar92@posteo.net> writes:

> Mehmet Tekman <mtekman89@gmail.com> writes:
>>
>> One issue that I cannot seem to solve by myself is getting the
>> `org-babel-tangle-sync-mode' to persist on the `after-save-hook'
>> after it's been activated.  My understanding of this hook is
>> that it is global and persists across buffers, but I'm seeing
>> some inconsistent behaviour that requires me to toggle the mode
>> on and off again.
>
> Any hook can be local.
> See LOCAL argument in help:add-hook

Ah, that solves the problem immediately thank you!

> Mehmet Tekman <mtekman89@gmail.com> writes:
>
>> I've modified the ob-tangle.el file for the main tangling and
>> detangling functions.  Most importantly, both functions can now
>> exchange information from the source Org mode file to the target
>> remote tangle file in either direction, depending on whether the
>> source Org file has `:tangle-sync <action>' in the header.
>
> Thanks!
>
>> The action is one of:
>>
>> - "export" = always transmit information from the source Org mode
>>              block to the target remote file.
>> - "import" = always transmit information from the target remote
>>              file to the source Org mode block.
>> - "skip" = skip the block.
>> - "both" = transmit information from source block to target block
>>            or target block to source, depending on whether the
>>            tangle or detangle is called from the source buffer or
>>            the target buffer respectively.
>
> May it be better to make :tangle header argument compound?
> Like ":tangle "file" export". Similar to :results header argument. See
> "16.6 Results of Evaluation" section of Org manual.
>

That's a great idea I had not considered, and would definitely reduce
the header bloat, especially since `:tangle-sync' *requires* the
`:tangle' header to be there.

> Also, some general comments on the patches:
>

Great!

> 2. When naming private functions, "--" should be after library prefix:
>    org-babel--...
>

Thanks, I will rename `org-babel-detangle-single-block--from-source'.

> 3. Please do not use private functions from third-party libraries. I am
>    talking about `cl--set-buffer-substring' in particular.
>

So initially I used `(setf (buffer-substring X Y) new-content)` but I
recieved a warning from Emacs that it was an obsolete generalized
variable.

After some searching I found this entry in an emacs fork used the cl
library:
  https://github.com/emacs-citar/citar/commit/809953a2191d0e3217ffbed9270be9b3cd6abfd2

Since `(require 'cl-lib)' is already imported in ~ob-tangle.el~, I did
not think it was too taboo to use. How does one then set the buffer
substring?


> 4. Please make sure that docstrings clearly describe what the function
>    does, each of its arguments, and the expected return value.
>    In the patch, `org-babel-detangle--block-contents', NEAREST is
>    ambiguous. The actual meaning is "block at point or previous block".
>
> 5. Pay attention to buffer boundaries. In particular, remember that
>    buffer may be narrowed when you call a command and that expressions
>    like (+ 2 (line-beginning-position)) may resolve beyond
>    `point-min'/`point-max'.
>
> 6. Avoid using cryptic list functions like `cdadar' when you can use
>    something more readable.
>
> 7. When naming functions or macros, make sure that the name is roughly
>    describing the purpose of the function. In `org-babel-detangle', you
>    added `single-block-metrics' local function that does not only do the
>    metrics, but also (unexpectedly!) calls
>    `org-babel-detangle-single-block'. This is especially important for
>    local functions that lack docstring.
>
> 8. In `org-babel-tangle', (setq block-counter (+ 1 block-counter))
>    appears to be misplaced into outer sexp level after your patch.

Thanks for these, I will clean up and better document my code.

> 1. Please make sure that patches do not leave Org in broken state in the
>    middle of the patchset. Your patch #1 adds two `defun's in the middle
>    of `org-babel-detangle', which is not right.
>
> 9. In commit messages, please mark newly added functions clearly.
>    Like "(org-babel-foo): New function. It does this and that."
>    Same for commit summaries - "lisp/ob-tangle.el: Detangle a single
>    block" is awkward. You should clearly indicate that you added
>    something new to the library.


Apologies. I rebased and squashed all my commits into one, and then
selectively staged hunks into seperate commits for the git format-patc
process.  For some reason the diff function decided that the new
functions should exist right in the middle of an existing function and I
was not sure how to resolve it at the time (though I have a better idea
now).

I will take better care with the messages. I tried to look for previous
"[ANN]" postings in the mailing list that I could emulate, but didn't
pay enough attention it seems.

I'm finally using `gnus' as my mail client so I'm slowly getting into a
more streamlined mindset that should be better at submitting and
formatting patches. (To reply to a mailing list, I do a wide reply to
the author and hope that the `Mail-Followup-To' header is used?)

Apropos patches:
Given how broken my current patches are, my next set of changes will be
not contingent on the previous ones. I will start a new set of patches.
I hope that's okay.

Best,
Mehmet