Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

create plus add-on #113

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

create plus add-on #113

wants to merge 2 commits into from

Conversation

driftcrow
Copy link

As discuss in #85 , I try to create an add-on for some features that have discussed. if you feel it's appropriate please accept .

because I don't know whether you will accept this , some code may roughly, you may modify them freely.

@nobiot
Copy link
Owner

nobiot commented Jan 4, 2022

@driftcrow

Thank you for putting together a PR.
There are a couple of comments:

  1. Org-transclusion is published on ELPA, which means that any contributor with more than 15 lines of code will need to have signed the FSF copyright paperwork.
    It's mentioned in Contributing section of README

    Org-transclusion is part of GNU ELPA and thus copyrighted by the Free Software Foundation (FSF). This means that anyone who is making a substantive code contribution will need to “assign the copyright for your contributions to the FSF so that they can be included in GNU Emacs” (Org Mode website).

    Have you done this in the past?

  2. What does it do? You mention part of the things dicussed in Some feature request  #85 was implemented. Which ones?
    I try to include a good user manual as part of the package. It's not perfect, of course, but I take time to maintain it. The current version is available online and as an Emacs Info doc (C-h i). Both are generated from the same source, which is this file in the repo. Would it be OK for you to also add a part to the manual, if we decide to include the plus extension in the package? It does not have to be now as there are other issues (point 1 above and 3 below)

  3. I installed org-transclusion-plus.el and tested transclusions in test-plus.org. I got an error saying I didn''t have file-exist-p! It's probably a macro from some other third party, or perhaps my Emacs version 29.0.50 does not have it. I cannot get the extension working.

Screenshot from 2022-01-04 18-08-19

@driftcrow
Copy link
Author

driftcrow commented Jan 5, 2022

oh, sorry for mistake. file-exists-p! is a macro define in my local config.
for question:

  1. I have not do that past. but if you feel this code appropriate ,you can modify anything and used with right method.

  2. My English is poor, so I can't explain my means clearly.
    this plug is enhance the flexibility of org-transclusion, user can use or define any custom function to preprocess the include file. in test file I show some scenes。 remove,keep lines with REGEXP; delete or insert anything with named src block define in doc; include self file ...

  3. file-exists-p! is a macro define in my local config in doom-emacs config , you can try with code below temporary (if need I can create a simply edition):

(defmacro file-exists-p! (files &optional directory)
  "Returns non-nil if the FILES in DIRECTORY all exist.

DIRECTORY is a path; defaults to `default-directory'.

Returns the last file found to meet the rules set by FILES, which can be a
single file or nested compound statement of `and' and `or' statements."
  `(let ((p ,(doom--resolve-path-forms files directory)))
     (and p (expand-file-name p ,directory))))


(defun doom--resolve-path-forms (spec &optional directory)
  "Converts a simple nested series of or/and forms into a series of
`file-exists-p' checks.

For example

  (doom--resolve-path-forms
    '(or A (and B C))
    \"~\")

Returns (approximately):

  '(let* ((_directory \"~\")
          (A (expand-file-name A _directory))
          (B (expand-file-name B _directory))
          (C (expand-file-name C _directory)))
     (or (and (file-exists-p A) A)
         (and (if (file-exists-p B) B)
              (if (file-exists-p C) C))))

This is used by `file-exists-p!' and `project-file-exists-p!'."
  (declare (pure t) (side-effect-free t))
  (if (and (listp spec)
           (memq (car spec) '(or and)))
      (cons (car spec)
            (mapcar (doom-rpartial #'doom--resolve-path-forms directory)
                    (cdr spec)))
    (let ((filevar (make-symbol "file")))
      `(let ((,filevar ,spec))
         (and (stringp ,filevar)
              ,(if directory
                   `(let ((default-directory ,directory))
                      (file-exists-p ,filevar))
                 `(file-exists-p ,filevar))
              ,filevar)))))

(defun doom-rpartial (fn &rest args)
  "Return a partial application of FUN to right-hand ARGS.

ARGS is a list of the last N arguments to pass to FUN. The result is a new
function which does the same as FUN, except that the last N arguments are fixed
at the values with which this function was called."
  (declare (side-effect-free t))
  (lambda (&rest pre-args)
    (apply fn (append pre-args args))))

@driftcrow
Copy link
Author

It is OK to close it if that is not what you want.

@nobiot
Copy link
Owner

nobiot commented Jan 6, 2022

I prefer to keep it open if you don’t mind, so that others may get inspiration.

I cannot merge it in the present form, though, partially for the reasons above.

I am still looking at the detail but I don’t think I should copy the doom prefixed function you use in the macro. I turned off the use-cache customising flag so that I didn’t need to use them. Then… It seems that plus creates a new buffer or file with uuid? It’s a bit of surprise.

But I think some ideas you presented are good. I am thinking of use of cache.

Thank you.

@driftcrow
Copy link
Author

Ok, this is just a demo for present some ideas ,and doom prefixed function is unnecessary。

because user operation is unpredictable,so create a copy of file for eval code with.

create with uuid is for cache. because I consider if use this method then next step can implementation allow transclusion multi level recurrence and enable support other format file link like: excel --covert to -->csv--->org file ... some operations with high delay in future.

so, thank you for you good package, and wish any idea is enlightening。

@devcarbon-com
Copy link
Contributor

This is interesting, thanks for keeping the pull request open, nobiot! I may take some inspiration from this going forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants