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

Fix: Revert de0b62b604e87ce43e2adc9e09bcd5174e8b877a (evil-with-delay change) #755

Merged

Conversation

gagbo
Copy link
Contributor

@gagbo gagbo commented Sep 17, 2023

EDIT: This PR now fully reverts the problematic commit, while adding some glue to make CI green.

Old version

This solution is incomplete, and might still fail because of the
impossibility to expand the hook-name dynamically, see the
discussion/screenshots in the PR

Brief summary of what the PR does

Try to address the issues in #750

Checklist

Assume you're working on mpc mode:

  • byte-compiles cleanly
  • M-x checkdoc is happy. Don't manually write (provide 'evil-collection-mpc), M-x checkdoc can do it automatically for you
  • define evil-collection-mpc-setup with defun
  • define evil-collection-mpc-mode-maps with defconst
  • All functions should start with evil-collection-mpc-

@gagbo
Copy link
Contributor Author

gagbo commented Sep 17, 2023

Edit

This comment is no longer relevant to the PR here, but mentions the former tries to use properly the new evil-with-delay macro to migrate (tl;dr: no way to have a dynamic hook-name without writing yet another macro layer on top of everything)

Old comment

I mostly tried to solve it by trying different quoting strategies and then using macroexpand-step to look at the result.

Before the PR (current state on master):
Screenshot_20230917_113458

Current state as the PR is made:
Screenshot_20230917_120301

Important notes

  • I still didn't test the fixes locally, it's a bit painful to do so yet, I mostly wanted to start a conversation about what we should expect the macro to expand to.
  • Note that in the before screenshot, the name of the hook is unique (evil-delay-in-{backquote}), which might hint that the issue is indeed that the quoting is off with the master version of evil-collection-translate calls.
  • Also note that I couldn't find a way to properly expand the value of hook-name in the resulting expansion, so I don't really know how to unlock this part. In a scratch buffer, calling macroexp-quote with a dynamic var seems to work:

image

So I'm not sure how I can go further and fix that. Is the change of evil-mode actually breaking after all? Or is there a quoting pattern that would fix this issue?

@gagbo gagbo force-pushed the fix/evil-collection-translation-delay branch from 95ce251 to d1ab9d9 Compare September 17, 2023 14:06
@gagbo gagbo changed the title Discussion/Draft: fix evil-with-delay quoting Fix: Revert de0b62b604e87ce43e2adc9e09bcd5174e8b877a (evil-with-delay change) Sep 17, 2023
On top of reverting the change, it relies on an intern backport of the
old evil-delay macro to avoid issues with CI.
@gagbo gagbo force-pushed the fix/evil-collection-translation-delay branch from d1ab9d9 to fe470f0 Compare September 17, 2023 14:12
@jojojames
Copy link
Collaborator

I think we should have a reverse commit of the original commit that caused the regression and then we add your fixes on top of the reverted commit. This way, we can evaluate the necessary changes from the original (deprecated) method and your new changes.

@gagbo
Copy link
Contributor Author

gagbo commented Sep 17, 2023

Probably. I just made the revert commit this way as a compromise to keep the CI green. If it's not important, just reverting is probably going to be enough.

I'm not well-versed in macro magic, but I had a lot of issues trying to get the custom evil-collection-translate-in-%s name in the new evil-with-delay macro.

@condy0919
Copy link
Collaborator

If there is no further discussion, I will merge 2 days later. :-)

@condy0919 condy0919 merged commit ac2862c into emacs-evil:master Sep 22, 2023
5 checks passed
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.

3 participants