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

Describe key #146

Merged
merged 22 commits into from
Nov 9, 2022
Merged

Describe key #146

merged 22 commits into from
Nov 9, 2022

Conversation

andyjda
Copy link
Contributor

@andyjda andyjda commented Nov 2, 2022

See describe-key-README for a detailed overview of approach and trade-offs.

All the relevant code should be at the bottom of god-mode.el.
Be sure to check the portion of the README that I added.

Let me know if you have any questions or if you want me to make any changes

god-mode.el Outdated Show resolved Hide resolved
god-mode.el Outdated Show resolved Hide resolved
god-mode.el Outdated Show resolved Hide resolved
god-mode.el Outdated Show resolved Hide resolved
god-mode.el Outdated Show resolved Hide resolved
@darth10
Copy link
Collaborator

darth10 commented Nov 4, 2022

Also, this branch needs to be rebased on master, as there are commits from previous PRs showing up.

@andyjda
Copy link
Contributor Author

andyjda commented Nov 7, 2022

  • rebased. conflicts are gone
  • removed custom variable god-translate-key-for-description
  • reverted indentation

See my comments about the other proposed changes. If you can think of a good way to deal with the hooks/functions, let me know. I am not too familiar with their underlying implementation: to me it looks like the current approach is the simplest, but if there's a better one I am down to work on it

Copy link
Collaborator

@darth10 darth10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting in those changes @andyjda!
I've tested out the god-mode-describe-key function and it's looking good!

I've left a few more comments around supporting older Emacs versions (< 28.1).

god-mode.el Outdated
But in our case it's redundant"
(insert
(format-message "\n %s\n %s%s\n %s%s%s\n\n"
"(`god-local-mode' is enabled. "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you're using a combination of tabs and spaces for indentation.
You should be using only spaces, as described in the Emacs Lisp style guide.
I'd recommend using M-x untabify on these new functions, or even the entire file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't know about the style, guide, thanks for letting me know!

god-mode.el Outdated
" The given key-sequence: "
(god-mode-get-described-key-seq)
" corresponds to this key-binding: "
(help--key-description-fontified
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

help--key-description-fontified was added in Emacs 28.1, but God mode supports Emacs 25.1+.
You'll need a wrapper function around this form that checks (> emacs-major-version 27) to support older Emacs versions.

Reference: emacs-mirror/emacs@4a112fd

god-mode.el Outdated
(condition-case err
(god-mode-lookup-key-sequence)
(wrong-type-argument
;; due to how errors are passed,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
;; due to how errors are passed,
;; due to how errors are passed,

god-mode.el Outdated Show resolved Hide resolved
god-mode.el Outdated Show resolved Hide resolved
.github/README.md Outdated Show resolved Hide resolved
@andyjda
Copy link
Contributor Author

andyjda commented Nov 9, 2022

I included all your proposed changes.
Let me know if there's anything else you'd like me to change. Thanks :)

Copy link
Collaborator

@darth10 darth10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for the contribution again @andyjda.
Closes #144.

@darth10 darth10 merged commit 49c1a17 into emacsorphanage:master Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants