I was a bit intrigued by the need to call a setup function in the init
file just to use this package. This has several disadvantages,
including being unusual and precluding lazy-loading the package. It's
also not clear what happens if the user forgets to call dtache-setup
before using the dtache commands.
Looking briefly at the definition of `dtache-setup`, it seems that the
`(unless dtache--sessions-initialized ...)` part can be called before
each dtache command, no?
As to the function added to `shell-mode-hook`, I'd suggest to let the
user do that manually in their config. One consideration here is that
comint can be a bit finicky, so, no matter how well designed dtache's
shell extension is, there could be bad interactions with other packages.
Augusto Stoffel <arstoffel@gmail.com> writes:
Hi Augusto,
> I was a bit intrigued by the need to call a setup function in the init> file just to use this package. This has several disadvantages,> including being unusual and precluding lazy-loading the package. It's> also not clear what happens if the user forgets to call dtache-setup> before using the dtache commands.
I agree that the current solution is a bit unorthodox, and also that it
causes some confusion when it comes to responsibilities.
I haven't given it a thought now for a while, if you have any ideas on
improvements to it I am all ears :)
> Looking briefly at the definition of `dtache-setup`, it seems that the> `(unless dtache--sessions-initialized ...)` part can be called before> each dtache command, no?
Yeah that part could probably be moved out, I guess the downside is that
it needs to be called from multiple other commands, but maybe that's
called for.
My previous thought was since dtache allows sessions that survives the
current Emacs session, or even runs on remote hosts, a common use case
would be to always want to run dtache on startup. It could be valuable
to be notified that a local/remote session has finished.
> As to the function added to `shell-mode-hook`, I'd suggest to let the> user do that manually in their config. One consideration here is that> comint can be a bit finicky, so, no matter how well designed dtache's> shell extension is, there could be bad interactions with other packages.
The shell-mode-hook is there, to be used independently of the way the
user wants to interface dtache. For those that only uses functionality
in `dtache.el` dtache-shell-command is the main interface, and
dtache-shell minor mode needs therefore to be activated.
So without the hook added all users would need to do that manually,
otherwise they couldn't use dtache-shell-command as it is intended. Or
do you see an other solution? :)
Hi Niklas,
this change looks good. You are just missing autoload cookies on
`dtache-shell-save-history-on-kill', `dtache-shell-override-history'
`dtache-org-babel-sh' (as well as any other function the user might want
to add to a hook or bind to a key that I might be missing here).
Incidentally, I'm not a use-package user but I wonder what these `:defer
t' and `:after shell' directives do and why an user would ever need to
worry about such subtleties. Maybe these things were necessary in the
past when autoloading was new and many packages didn't do it properly.
In any case, without use-package (and after you add the autoloads I
mentioned above), setting up the package is just:
(add-hook 'shell-mode-hook 'dtache-shell-mode)
(global-set-key [remap async-shell-command] 'dtache-shell-command)
(setq dtache-show-output-on-attach t)
(advice-add 'shell :around #'dtache-shell-override-history)
(add-hook 'shell-mode-hook #'dtache-shell-save-history-on-kill)
(setq dtache-shell-history-file "~/.bash_history")
and similarly from the other bits.
Augusto Stoffel <arstoffel@gmail.com> writes:
> Hi Niklas,>> this change looks good. You are just missing autoload cookies on> `dtache-shell-save-history-on-kill', `dtache-shell-override-history'> `dtache-org-babel-sh' (as well as any other function the user might want> to add to a hook or bind to a key that I might be missing here).
Hi again,
thanks for clarifying this. I think I hadn't fully grasped the concept
of the autoload cookies I realized.
I went one step further and introduced a dtache-init.el file. Which
purpose should be to setup the integration with the other
packages. I want the users to not have to have as much boilerplate code
as it has been up to now. Do you think this is a good approach?
https://git.sr.ht/~niklaseklund/dtache/tree/rename/detached-init.el#L106
The use-package example then becomes much smaller as well:
https://git.sr.ht/~niklaseklund/dtache/tree/rename/README.md#L51> Incidentally, I'm not a use-package user but I wonder what these `:defer> t' and `:after shell' directives do and why an user would ever need to> worry about such subtleties.
That's great, I have only used Emacs with use-package so I appreciate
feedback about that. You put the finger on flaws here.
> Maybe these things were necessary in the> past when autoloading was new and many packages didn't do it properly.> In any case, without use-package (and after you add the autoloads I> mentioned above), setting up the package is just:> (add-hook 'shell-mode-hook 'dtache-shell-mode)> (global-set-key [remap async-shell-command] 'dtache-shell-command)> (setq dtache-show-output-on-attach t)>> (advice-add 'shell :around #'dtache-shell-override-history)> (add-hook 'shell-mode-hook #'dtache-shell-save-history-on-kill)> (setq dtache-shell-history-file "~/.bash_history")>> and similarly from the other bits.
Much better than before :). I think the new implementation should align
better with what you describe. Also I have provided links to a branch
that I am currently working on where dtache is renamed to detached.el
just FYI.
/Niklas