search

emacs-evil / evil-surround

you will be surrounded (surround.vim for evil, the extensible vi layer)
Other
631 stars 61 forks source link

Fix some warnings in tests and a small optimization #208

Closed Hi-Angel closed 7 months ago

tomdl89 commented 9 months ago

Thanks @Hi-Angel I ran the tests locally to ensure this didn't break anything, and the tests failed to run because they depended on a since-changed evil function. I fixed this and pushed to your branch. But more broadly, I think changes like your should be well tested before merging. I'm not confident the test suite is comprehensive enough, so please confirm you have test-driven this branch before we merge. While I agree that moving to lexical scope is better, I do think working code is more important than correctness, so would rather be cautious here. Cheers

Hi-Angel commented 9 months ago

Thanks, sure, I can use it locally for some time before it's merged.

FTR, I tried running the tests locally before creating a PR, but they didn't work (probably due to the problem you fixed). I kind of hoped they will work as part of CI once I create the PR.

tomdl89 commented 9 months ago

Yeah, I'd definitely like to get CI working for this repo. It's just a bit of work. Probably just need to do what I did for evil-cleverparens recently: https://github.com/emacs-evil/evil-cleverparens/commit/af7dcc27148aab383147146f37f6808a3943060f and https://github.com/emacs-evil/evil-cleverparens/commit/a38fe3235b10096d1a94790f34682069ee87bbda I'll test-drive too for a bit. Hopefully we can merge soon :)

Hi-Angel commented 8 months ago

So, is everything seems fine?