search

emacs-evil / evil

The extensible vi layer for Emacs.
GNU General Public License v3.0
3.39k stars 280 forks source link

Move `evil-test-helpers.el` to its own repo? #1882

Closed jcs090218 closed 18 hours ago

jcs090218 commented 7 months ago

Opting for a single package per repository appears to be a preferable option, as indicated in melpa/melpa#8172. This approach also streamlines the process of conducting CI tests and enhances clarity regarding the association between packages. 🤔

tomdl89 commented 7 months ago

@jcs090218 before I answer specifically to this issue, I should preface by saying I'm not such a big fan of purely "correctness" changes to established projects like evil. But I'm happy to be convinced when it's more than just correctness.

Specifically to this issue, I read https://github.com/melpa/melpa/issues/8172 and I'm not sure the presence of evil-test-helpers.el in evil's repo is particularly relevant. That policy is about separate packages being in all-in-one repos, but I think it's too much of a stretch to say evil-test-helpers.el is a separate package. I'm not aware of it being required by users or other packages in a scenario where at least some part of the rest of evil isn't required.

The sell that it "also streamlines the process of conducting CI tests" sounds alluring, but what do you actually mean? Can you be more concrete? When I think about it, I'm not sure I see much in the way of practical streamlining. Maybe I'm missing something. And on your last point, I get the basic idea that splitting things out enhances clarity, but like I said, I don't really think they're separate packages, so I'm not sure why we need to "enhance clarity" between them..?

jcs090218 commented 7 months ago

The maintainer(s) are not interested. Closing this issue then. :)

tomdl89 commented 7 months ago

I should reiterate that I welcome counter-arguments to my points. I'm not immediately "interested" insofar as wanting to make the change without further discussion. But I am "interested" insofar as being curious about arguments I hadn't considered. I hope you don't feel I've been overly dismissive.

jcs090218 commented 7 months ago

I should reiterate that I welcome counter-arguments to my points. I'm not immediately "interested" insofar as wanting to make the change without further discussion. But I am "interested" insofar as being curious about arguments I hadn't considered. I hope you don't feel I've been overly dismissive.

Sorry, I shouldn't have gone to the conclusion that quickly. I apologize. I will elaborate more below.

The sell that it "also streamlines the process of conducting CI tests" sounds alluring, but what do you actually mean? Can you be more concrete? When I think about it, I'm not sure I see much in the way of practical streamlining. Maybe I'm missing something.

I was planning to contribute to this project by first replacing Cask with Eask since Cask has been deprecated for a long time. I would then clean up the Makefile and add tests for macOS and Windows. Similar to the PR https://github.com/emacs-evil/evil-collection/pull/804.

And on your last point, I get the basic idea that splitting things out enhances clarity, but like I said, I don't really think they're separate packages, so I'm not sure why we need to "enhance clarity" between them..?

I was a bit confused by this since evil-test-helper itself has its own recipe on MELPA, so I assumed they are two separate packages... no? 🤔

Other than that, having tests in the test folder would help clarify what package files and test files are. Should evil-tests.el be moved into the test folder?

tomdl89 commented 7 months ago

Thanks @jcs090218 . I read https://github.com/emacs-evil/evil/issues/846 and agree with @wasamasa 's comment:

I'd be in for this change, as long as evil-tests-helpers.el remains in this repository so that Evil itself doesn't need to do anything else than (require 'evil-tests-helpers) in its test file.

So it looks like they decided to make evil-test-helpers its own package, but they let the recipe point to evil's repo, so they are always in sync. As they concluded, I think keeping it in the evil repo is the right thing to do. I'm still not aware of any arguments to do otherwise.

With regard to moving evil-tests.el and evil-test-helpers.el to a test folder, I'm not strongly opposed to it, but I don't think it affects clarity that much either way. If you want to make a PR, I would probably merge it.

I was planning to contribute to this project by first replacing Cask with Eask since Cask has been deprecated for a long time. I would then clean up the Makefile and add tests for macOS and Windows. Similar to the PR https://github.com/emacs-evil/evil-collection/pull/804.

I don't think we actually use Cask, but I do in general like the idea of us moving to something like Eask. I introduced Eldev to evil-cleverparens and I do like it. Evil's current approach is a bit too "hand-rolled" for my liking. PR welcome on this.

jcs090218 commented 7 months ago

So it looks like they decided to make evil-test-helpers its own package, but they let the recipe point to evil's repo, so they are always in sync. As they concluded, I think keeping it in the evil repo is the right thing to do. I'm still not aware of any arguments to do otherwise.

Thanks for clarifying this! :)

With regard to moving evil-tests.el and evil-test-helpers.el to a test folder, I'm not strongly opposed to it, but I don't think it affects clarity that much either way. If you want to make a PR, I would probably merge it.

👍

I don't think we actually use Cask, but I do in general like the idea of us moving to something like Eask. I introduced Eldev to evil-cleverparens and I do like it. Evil's current approach is a bit too "hand-rolled" for my liking. PR welcome on this.

Oh, sorry. I saw the Cask file, so I assumed this package was using Cask. 😓

Eldev is another good choice, but Windows support is a second-class citizen. As the author of Eask, I treat all modern OSs as first-class citizens. 🤔 BTW, evil-cleverparens looks very interesting! 😋