Evil Integration

[jojojames]

@noctuid @Ambrevar

Would love some input on this.

The defcustom from evil's 'evil-want-integration' is a little awkward to use so we either need to put it in the readme for users to disable or figure out how to disable it from evil-collection. Either way, it seems messy.

This won't pass the byte compiler (since there's a few warnings around unused stuff) yet.

Having to rename some of those variables/functions to pass the linter is also regrettable and makes me think this might be tedious to track upstream's evil-integration when/if it changes.

At the very least, with the dired removals, we stop seeing duplicate dired menus.!!

[jojojames]

Will merge this soon unless there are issues.

[noctuid]

I think that it would be preferable if all the keybindings for major modes (and any mode-specific stuff) wer moved to the corresponding file. That can be done later though if you want to.

The (or..) for the message isn't needed; you can just have (bound-and-true-p..). Also, for the use-package example, I think it might be preferable to use :init to make it clear that it needs to be set before loading evil (as well as change the explanation to "Make sure to set ~evil-want-integration~ to nil before loading evil or evil-collection").

[jojojames]

I think that it would be preferable if all the keybindings for major modes (and any mode-specific stuff) wer moved to the corresponding file. That can be done later though if you want to.

Yup. I plan that as a separate step.

The (or..) for the message isn't needed; you can just have (bound-and-true-p..).

The intent there was to also guard against the case where it wasn't bound. I was tracking evil's head for a while.. But upon closer thought, you're right, it doesn't need to be bound-checked so I'll remove that part.

Also, for the use-package example, I think it might be preferable to use :init to make it clear that it needs to be set before loading evil

Sounds good, I'll give that a shot then.

[fbergroth]

I think this can simply check for (featurep 'evil-integration) after the (require 'evil).

[jojojames]

featurep check is OK with me though it does have a little bit of indirection. :)

[jojojames]

I wonder if it makes sense just to throw a user-error if it isn't set to nil (evil-want-integration)/(or if the feature is provided) instead of messaging the user and hoping they fix it.

Thoughts @noctuid @fbergroth @Ambrevar ?

[noctuid]

I think it would be better to keep it as a message so it doesn't potentially halt/break init. As for using featurep, you may also want to check evil-want-integration and message that it wasn't set before loading evil if it is nil and evil-integration was still loaded.

[jojojames]

you may also want to check evil-want-integration and message that it wasn't set before loading evil if it is nil and evil-integration was still loaded.

I'm trying to grok this sentence and I feel like I understand but also that I don't. :)

[noctuid]

Something like this:

(if (featurep 'evil-integration)
    (if evil-want-integration
        (message
         "Make sure to set `evil-want-integration' to nil before loading evil or evil-collection.")
      (message "`evil-want-integration' was set to nil but not before loading evil."))
  (require 'evil-collection-integration))

[fbergroth]

I would like to see evil-collection-integration being split into subpackages like the rest of evil-collection. It will probably be hard to try to maintain compatibility with the old evil-integration, so I'd be in favor for making the message a user-error.

[jojojames]

@noctuid Thanks for spelling that out.

--NVM: See the edit.--

Side comment: Is there a way to wrap these message strings without using concat/format but also going under the 80 column limit?

[jojojames]

I would like to see evil-collection-integration being split into subpackages like the rest of evil-collection.

Yup, I'll start splitting them out soon after this PR is merged.

It will probably be hard to try to maintain compatibility with the old evil-integration, so I'd be in favor for making the message a user-error.

Yeah, I'm planning on tagging the latest evil commit in any Integration: commit so that it's easier to port changes over but I do think it'll be somewhat tedious.

As for user-error vs message, I was leaning towards user-error but I'd like more votes on either choice before going for either. I don't have a strong opinion here.

@dieggsy @edwargix @Somelauw

Feel free to throw in your 2c too.

[noctuid]

Side comment: Is there a way to wrap these message strings without using concat/format but also going under the 80 column limit?

You can do this but it's not pretty either:

(message "Some \
text")

[Somelauw]

I'm wondering if it would be reasonable for a user to leave evil-want-integration set to t when they are only interested in one of more specific submodules of evil-collection that don't conflict with evil-integration. Even if we recommend against it, I don't know if we should forbid this by throwing an error.

As for user-error vs message, I was leaning towards user-error but I'd like more votes on either choice before going for either. I don't have a strong opinion here.

As a middle ground, display-warning might also be an option.

[jojojames]

I'm wondering if it would be reasonable for a user to leave evil-want-integration set to t when they are only interested in one of more specific submodules of evil-collection that don't conflict with evil-integration.

It'd probably be very tricky once all the parts split into their separate modules. It's probably best to strongly recommend disabling it altogether.

Even if we recommend against it, I don't know if we should forbid this by throwing an error.

Sounds good.

As a middle ground, display-warning might also be an option.

I'm going with that option for now. It's a good middleground between 'in your face' and 'doesn't break init'.