svaante / dape

Debug Adapter Protocol for Emacs
GNU General Public License v3.0
455 stars 25 forks source link

Move setting dape-key-prefix to use-package ":preface", add gud pk #69

Closed jeff-phil closed 6 months ago

jeff-phil commented 7 months ago

Simple change to README.org.

svaante commented 7 months ago

Nice! This is definitely an improvement.

I have a couple of thoughts/questions?

Why :preface over :init?

I don't think this change is necessary, I think the original heads up that it mentions the gdb keybinding is fine but also wrong as it should be ";; By default dape shares the same keybinding prefix as `gud'"

If I was not as familiar with the dape' package I would assume that gud-key-prefix' value had something to do with the functionally of dape.

jeff-phil commented 7 months ago

Hi!

Why :preface over :init?

With use-package you can think of the simplified steps as :preface -> ('require foo.el) -> :init -> :config.

It looks like with both dape: (when dape-key-prefix (global-set-key dape-key-prefix dape-global-map)), and gud: (global-set-key gud-key-prefix gud-global-map); keymap prefix definitions are free hanging which would cause them to be set during loading of the package, aka during the require. This means to override, it would need to be set prior such as in then :preface section. Setting in :init is already too late.

Ideally, I believe this would normally be done or called in the mode's definition. But since both dape and gud have multiple modes, not sure that is doable with a lot more work that won't add much vs. just documenting to set keymap prefixes in :preface.

I don't think this change is necessary, I think the original heads up that it mentions the gdb keybinding is fine but also wrong as it should be ";; By default dape shares the same keybinding prefix as gud'" ;; By default dape uses gdb keybinding prefix If I was not as familiar with thedape' package I would assume that `gud-key-prefix' value had something to do with the functionally of dape.

I don't disagree. But it is a bit of a chicken and egg, since dape is using some of gud's functionality there is definitely a tie there. And since dape requires gdb-mi, then it's going to override keymaps if not set and people won't know where it's coming from. So was more of a information "convenience" to the users.

One option may be for dape to go ahead and behind the scenes set gud-key-prefix before gdb-mi is included.

(setq-local gud-key-prefix "")
(require 'gdb-mi)

Since, by default if uses C-x C-a prefix, dape will wipe out it's prefix map anyway with (global-set-key dape-key-prefix dape-global-map). This again, may get confusing for anyone using both dape and gud. Not sure who that would be.

If I may also add, though, since dape has dependency of emacs 29.1, setting keymap prefixes and keymaps this way anymore isn't needed and already deprecated. That doesn't help gud today, but setting dape up for long-term - may want to consider future-proofing now.

Again, just trying to cut down on confusion with a simple 1-line comment, vs. the rabbit hole of dependency hell.

TL;DR... I'm okay taking out the extra gud comments. Just let me know. :)

svaante commented 6 months ago

Sorry for taking some time to respond, please change to:

;; By default dape shares the same keybinding prefix as `gud'

And https://github.com/svaante/dape/pull/5#issuecomment-1872517008

Then we are good to go.

jeff-phil commented 6 months ago

Thanks! Should be all set.