emacs-orgmode
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [O] [rfc] org-dired


From: Nicolas Goaziou
Subject: Re: [O] [rfc] org-dired
Date: Wed, 15 Nov 2017 12:38:37 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux)

Hello,

Marco Wahl <address@hidden> writes:

> I just pushed the functionality to master.


Thank you. 

However, I didn't have time to comment the code.

There are a few stylistic issues:

  `mapc' + `lambda' -> `dolist' in `org-attach-attach-files'.  

However, I think `org-attach-attach-files' can be removed, since it is
called only once and is really two lines long. IOW, please include it in
`org-attach-dired-attach-to-next-best-subtree'.

  "no window in Org-mode" -> "No window displaying an Org buffer"

I don't think it is useful to implement
`org-attach-dired-attach-to-next-best-subtree-mv'. I assume that once
`org-attach-method' is set, a user is unlikely to change it for a single
command. IOW, let's just implement
`org-attach-dired-attach-to-next-best-subtree'.

Nitpick: an inline comment uses a single semicolon.

It would also be better to shorten function names, e.g.

  org-attach-dired-attach-to-next-best-subtree  ->  org-attach-dired-to-subtree


There are also a few issues in "test-org-attach.el".

For example `touch' uses the wrong name-space. Besides, it is not
useful. We usually do

  (org-test-with-temp-text-in-file 
    "... Org bufer..."
    (let ((filename (buffer-file-name)))
     ...))

It would be best to refactor
`test-org-attach/dired-attach-to-next-best-subtree/1' so that `should'
is the outer sexp. The cleanup part should probably be in an
`unwind-protect'. Not that it is not useful if
°org-test-with-temp-text-in-file'.

Regards,

-- 
Nicolas Goaziou



reply via email to

[Prev in Thread] Current Thread [Next in Thread]