astoff / devdocs.el

Emacs viewer for DevDocs
279 stars 16 forks source link

Add use-package keyword for managing docs. #12

Closed canatella closed 2 years ago

astoff commented 2 years ago

Can you explain what is the benefit of this change?

canatella commented 2 years ago

Oh sorry it is in the README.md diff:

** Use package

If you use use-package, a =devdocs= keyword is provided that will ensure
documentation is installed and configure the =devdocs-current-docs= variable.

#+begin_src elisp
(use-package python :devdocs (python-mode "python~3.9"))
#+end_src

This will call =devdocs-install= to install the =python~3.9= documentation if it
is not already installed and add a hook to set =devdocs-current-docs= to
='("python~3.9")= for =python-mode=.

and for reference: https://github.com/jwiegley/use-package (it is quite popular nowadays and the keyword is also supported by the straight package manager with this code too. The goal of all this is that when cloning a config to a new environment, the devdocs are re-installed along their respective package.

astoff commented 2 years ago

Thanks for the PR and sorry for the delay to reply.

I agree a function like devdocs-ensure-installed is necessary (but its signature should be &rest docs; also, I prefer to modify devdocs-install so that the argument can be a string as well as a doc-alist).

Concerning the use-package keyword, I'm reluctant to include it (although I see the appeal), in part because I don't use use-package myself, so it would be hard for me to maintain the code. Is there a different place to distribute package-specific use-package extensions?

In any case, I agree that the suggestion from the current README to copy something like

(add-hook 'python-mode-hook
          (lambda () (setq-local devdocs-current-docs '("python~3.9"))))

to your init file is not great, and we can try to make the package easier to configure with regular Elisp.

Things could be improved a bit by introducing a variable, say devdocs-major-mode-docs, which would be an alist mapping major modes to lists of documents. Then one can say

(add-to-list 'devdocs-major-mode-docs '(python-mode "python~3.9" "pytorch")

either inside a (with-eval-after-load 'devdocs ...) or inside a use-package form. Do you think this helps/is good enough?

minad commented 2 years ago

Fwiw I don't think this should be included here. I think having this in a separate package is better. I am also not a use-package user anymore.

canatella commented 2 years ago

I'd like the Emacs ecosystem to not go the npm way with 1 package for 10 lines of code. This is provided as a separate file that you don't need to require if you don't use use-package. According to melpa, use-package has been downloaded 1,750,425 times, it has 238 forks and 3800 stars on github, so they are people using it. Also this works with the straight package manager too. I'm open to any other ways of implementing it, but best is that it stays if possible near the main functionality.

minad commented 2 years ago

I'd like the Emacs ecosystem to not go the npm way with 1 package for 10 lines of code.

I agree with that. But integration packages like this are best kept separately from the main package. First it is a matter of maintenance and second while use-package is widely used, it is still not the one and only solution everybody is using. I used it myself but moved away from it due to its complexity. I have a handful of my own macros in my config to facilitate lazy loading and keybindings and everything works equally well without the additional layer. (But I admit that this is NIH, use-package is a fine tool!)

I think what @astoff is proposing here is good - providing better means to configure devdocs while relying purely on builtins, such that the facilities are not use-package specific.

astoff commented 2 years ago

@canatella I share your opinion about tiny packages, and I've argued in the past in favor of not splitting some integration code into separate packages. Consult includes integrations for selectrum and icomplete which are just sitting in my hard drive and don't bother me at all.

But in the present case, I feel this is not the right approach. Even the file name, use-package-devdocs.el, suggests it belongs somewhere else (and I'm afraid I have no good suggestion as to where).

More critically though, since that file depends on use-package-core, it gives a compilation error on my Emacs.

canatella commented 2 years ago

This file add devdocs support to use-package. There are many more user of use-package then user of devdocs so it make sense to me to put the integration on the devdocs side. Also use-package is considered for inclusion in emacs itself while devdocs is not. I could rename the file to devdocs-use-package.el to make that more clear. The compilation error can probably be fixed with an eval after load. I'm willing to try and test this, but only if this is going to be merged. If you think its not worth it, so be it, but its a pity for all the use-package users out there, while its 40 lines of code.

minad commented 2 years ago

@canatella According to the MELPA maintainers it is recommended to keep integration packages separately. I agree with this perspective, this is what I recommend in this case and this is what I do with my packages. There is precedent, there exist already other use-package-* packages on MELPA.

Since @astoff mentioned the Consult integration of completion UIs - this is true, but it is a bad example. I support completion UIs directly in the Consult package for technical reasons. Originally I distributed a separate consult-selectrum package etc, but it turned out too error prone that way. Many users forgot to install that package leading to a half-functioning Consult package. Also Consult is designed in a way which reduces the amount of integration code to an an absolute minimum. I refuse to add new functionality to Consult which would require an extension of the integration code. This exception is only made for the integration code for completion UIs, since Consult wouldn't function correctly without the integration code.Therefore these small integrations are an exception to the rule to keep integration code separately. The decision to include the integration code has not been made lightly, only after careful consideration and after bad experience with the earlier approach.

Note that other Consult-integration packages are distributed separately: consult-flycheck, consult-yasnippet, embark-consult, ... Each of those packages provides additional functionality which you have to install explicitly, to opt-in to the additional features.

Such a technical argument cannot be made for use-package integration of devdocs.el!

canatella commented 2 years ago

Alright, I'll close the PR then. Thanks for taking the time to go through this!

minad commented 2 years ago

Alright, I'll close the PR then.

I am not the maintainer. I just stated my experiences. It is up to @astoff to decide on how to proceed here. I am not interested in demotivating you. I do think that your package is valuable for use-package users and that it should be distributed. Maybe there is a point in collecting use-package integrations in a use-package-contrib package, similar to how it is done in the evil-collection?

astoff commented 2 years ago

Hi @canatella,

The compilation error can probably be fixed with an eval after load.

This would introduce warnings about unknown functions or variables; and fixing that is not impossible, but too much of a hack.

So I very much agree with @minad here, in particular about the fact that the functionality of this PR is valuable and should be distributed through the appropriate medium.

Evil-collection also crossed my mind while thinking about this. I don't know much about it, but I know that a user added devdocs integration there. There's also the no-littering package that provides "batch customizations" for various packages. It looks like a similar initiative is in order for use-package.

And while I'm at it, let me repeat one of my comments from above. I'm not convinced it's a good idea, but maybe someone has an opinion:

Things could be improved a bit by introducing a variable, say devdocs-major-mode-docs, which would be an alist mapping major modes to lists of documents. Then one can say

(add-to-list 'devdocs-major-mode-docs '(python-mode "python~3.9" "pytorch")

either inside a (with-eval-after-load 'devdocs ...) or inside a use-package form. Do you think this helps/is good enough?