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

Various code cleanups #1693

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Various code cleanups #1693

wants to merge 2 commits into from

Conversation

tomdl89
Copy link
Member

@tomdl89 tomdl89 commented Oct 5, 2022

The original motivation was to fix the misuse of eval in evil-delay by turning it into a macro (name evil-with-delay). But it includes various generic changes such as prefering #' to quote function names and fixing some incorrect uses of ' in docstrings (many warnings remain about this).

The patch also enables lexical-binding in the remaining files. lexical-binding in evil-ex.el has had a tumultuous life, the last commit of which sets it explicitly to nil while stating confusingly in the commit message that it re-enables it. In any case, I still found some changes needed to account for lexical-binding, so there might be more.

Detailed changes below.

  • evil-common.el (evil-unquote): Delete function, not used (luckily: it reeked of a bad hack to work around a misunderstood bug). (evil--with-delay): New helper function.
    (evil-with-delay): New macro to replace evil-delay. (evil-delay): Rewrite using evil-with-delay and mark as obsolete. (evil-signal-at-bob-or-eob): Fix typos in docstring.

  • evil-core.el (window-configurakion-change-hook): Add FIXME. (evil-define-key): Use evil-with-delay.

  • evil-states.el (evil-visual-activate-hook): Use evil-with-delay.

  • evil-commands.el (evil-match): Use pcase since the branch patterns used are those of pcase rather than those of cl-case. (evil-execute-in-normal-state): Use evil-with-delay.

  • evil-digraphs.el (evil-digraphs-table-user): Remove redundant :group arg.

  • evil-ex.el: Enable lexical-binding like the last commit that touched this cookie said that it was doing (even though it didn't). (evil-ex-info-string): Declare.
    (evil-ex-update): Mark end and len as ignored. (evil-ex-init-shell-argument-completion): Mark arg as ignored. (evil-flatten-syntax-tree): Mark char as ignored. (evil-parser): Rename context to evil--context and declare it as dynbound. Remove unused var last.
    Move shared setq result out of some ifs and conds.

  • evil-jumps.el: Remove redundant :group` arguments.

  • evil-macros.el (evil-define-interactive-code): Move shared setq func out of cond. Move the insertion of quote around func to the cond so the quote is not incorrectly added around lambda forms.

  • evil-pkg.el: Remove file. Move its contents to the pseudo headers of evil.el so (M|NonGNU)ELPA can auto-generate this file appropriately.

  • evil.el: Enable lexical-binding. Synchronize metadata with what was in evil-pkg.el.

  • evil-tests.el: Enable lexical-binding. (evil-test-change-state): Move let to obviate the need for setq. Remove unused vars keymap and local-keymap.
    (evil-test-auxiliary-maps): Rename map to evil--map and declare it as dynbound so evil-define-key can access it.
    (evil-test-exclusive-type): Mark third-line as unused. (evil-test-text-object): Mark type arg as unused. (evil-with-both-search-modules): Move macro before its first use. (evil-test-properties): Rename alist to evil--alist and declare it as dynbound so evil-put-property can access it.

  • evil-command-window.el (evil-command-window-draw-prefix): Mark ignored as, well, ignored.

Closes #1685

monnier and others added 2 commits January 11, 2023 20:23
The original motivation was to fix the misuse of `eval` in `evil-delay`
by turning it into a macro (name `evil-with-delay`).  But it includes
various generic changes such as prefering #' to quote function names and
fixing some incorrect uses of ' in docstrings (many warnings remain about
this).

The patch also enables `lexical-binding` in the remaining files.
`lexical-binding` in `evil-ex.el` has had a tumultuous life, the last commit
of which sets it explicitly to nil while stating confusingly in the commit
message that it re-enables it.  In any case, I still found some changes
needed to account for `lexical-binding`, so there might be more.

Detailed changes below.

* evil-common.el (evil-unquote): Delete function, not used (luckily:
it reeked of a bad hack to work around a misunderstood bug).
(evil--with-delay): New helper function.
(evil-with-delay): New macro to replace `evil-delay`.
(evil-delay): Rewrite using `evil-with-delay` and mark as obsolete.
(evil-signal-at-bob-or-eob): Fix typos in docstring.

* evil-core.el (window-configurakion-change-hook): Add FIXME.
(evil-define-key): Use `evil-with-delay`.

* evil-states.el (evil-visual-activate-hook): Use `evil-with-delay`.

* evil-commands.el (evil-match): Use `pcase` since the branch patterns
used are those of `pcase` rather than those of `cl-case`.
(evil-execute-in-normal-state): Use `evil-with-delay`.

* evil-digraphs.el (evil-digraphs-table-user): Remove redundant
`:group` arg.

* evil-ex.el: Enable `lexical-binding` like the last commit that
touched this cookie said that it was doing (even though it didn't).
(evil-ex-info-string): Declare.
(evil-ex-update): Mark `end` and  `len` as ignored.
(evil-ex-init-shell-argument-completion): Mark `arg` as ignored.
(evil-flatten-syntax-tree): Mark `char` as ignored.
(evil-parser): Rename `context` to `evil--context` and declare it
as dynbound.  Remove unused var `last`.
Move shared `setq result` out of some `if`s and `cond`s.

* evil-jumps.el: Remove redundant  :group` arguments.

* evil-macros.el (evil-define-interactive-code): Move shared `setq func`
out of `cond`.  Move the insertion of quote around `func` to the `cond`
so the `quote` is not incorrectly added around lambda forms.

* evil-pkg.el: Remove file.  Move its contents to the pseudo headers of
`evil.el` so (M|NonGNU)ELPA can auto-generate this file appropriately.

* evil.el: Enable `lexical-binding`.  Synchronize metadata with what
was in `evil-pkg.el`.

* evil-tests.el: Enable `lexical-binding`.
(evil-test-change-state): Move `let` to obviate the need for `setq`.
Remove unused vars `keymap` and `local-keymap`.
(evil-test-auxiliary-maps): Rename `map` to `evil--map` and declare it
as dynbound so `evil-define-key` can access it.
(evil-test-exclusive-type): Mark `third-line` as unused.
(evil-test-text-object): Mark `type` arg as unused.
(evil-with-both-search-modules): Move macro before its first use.
(evil-test-properties): Rename `alist` to `evil--alist` and declare it
as dynbound so `evil-put-property` can access it.

* evil-command-window.el (evil-command-window-draw-prefix):
Mark `ignored` as, well, ignored.
Suggested cleanups:

* Remove commented dead code, e.g. keymap testing was moved to
  evil-test-state-keymaps.
* Implicitly use current buffer with buffer-file-name.
* vi should probably be capitalized as vi.
@axelf4
Copy link
Collaborator

axelf4 commented Jan 11, 2023

@monnier I rebased this onto master and pushed a commit that fixed the tests and added some optional suggestions. Would be great to get this merged. Two comments:

@monnier
Copy link
Contributor

monnier commented Jan 14, 2023 via email

@monnier
Copy link
Contributor

monnier commented Jul 3, 2023

I pushed to the scratch/evil branch of git://git.sv.git.org/emacs/nongnu.git a reworked version of this patch, split into various commits so it's easier to merge it piecemeal. It also refrains from using #' at those places where it would currently end up adding a "spurious" warnings (fixing those warnings is currently rather inconvenient because of the circular dependencies we have between files).
It also adds one important change which is to replace the use of defadvice with advice-add.

@Hi-Angel
Copy link
Contributor

For the readers, the review for this PR per my understanding is happening here #1819

I also stumbled upon this PR because upstream (not yet released) Emacs has deprecated defadvice, so would nice to have this fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Patch to enable lexical-binding and improve evil-delay
4 participants