Use transient.el

[mathrick]

This updates magit-p4 to use transient instead of magit-popup.

Summary of changes:

• Code now uses transient-define-prefix instead of magit-define-popup, alongside with all the other changes to accommodate the differences between the two libraries
• Keys used have been updated slightly to align them better with keys used by native magit transients for conceptually similar operations
• Bind 4 to magit-p4-popup in the top-level magit keymap, so it can be invoked without going through magit-dispatch. This currently conflicts with the binding for magit-section-show-level-4, but I've personally never used that one, unlike magit-section-show-level-4-all, so this seems like a worthy sacrifice

[tarsius]

(I am switching to "regular posts" because I want to use Forge and that doesn't support "review comments" yet.

Just to make sure I understand what you're saying properly, the code as I have it right now is correct, right?

No I was trying to say that you should byte-compile to figure out whether you have to use eval-and-compile. I have done that now and the answer is yes.

I have started by adding a Makefile, which allows me to run the compiler. (It follows the same basic format as I use for all my packages. It assumes that all dependencies can be found in git repositories at ../<name>. If that doesn't work for you you can adjust the LOAD_PATH in config.mk.

IMO that's the first step when taking over an existing package: make sure you can build it, so that you can find and address existing issues and to some extend avoid regressions. (Only after that is complete should one move on to improvements, i.e., your switch to transient should not have come first.)

So I went on to address existing issues. Since I mentioned what you should have done differently, I'll also admit that addressing these issues myself was "wrong". I should have guided you through the process, but that would have taken more time, and I have so many other things to do.

But I left something for you to do: I am not sure my change to magit-p4-clone (in 454341c * Enable lexical-binding and fix a bug thus revealed) is correct. Please check that.

Finally I have rebased your changes on top of my cleanup.

I think it would be better if you squashed your commits, and my fixup commit, into a single commit. Your original plan to split up the work in "basic changes" and "further improvements" is good, but when it turned out that the first of these had issues (the transient-args thing), that shouldn't be addressed in a new commit. The original commit should be fixed instead. Maybe your plan was to do that later and just added a new commit to make it easier for me to review. Other maintainers will appreciate that, but I prefer when commits are fixed immediately. I've you wish you can keep the two original commits, but that's more work because you'll have to resolve more conflicts during rebase, so I won't insist on that.

(Edit: From a quick look you only have to fixup the second of the original commits. In that case it is probably best and easy to keep using two commits. But if you do that, please make sure the intermediate step also compiles without errors.)

[tarsius]

Please open a pr in their repository to address these:

../p4/p4.el: Warning: ‘loop’ is an obsolete alias (as of 27.1); use ‘cl-loop’ instead.
../p4/p4.el: Warning: ‘loop’ is an obsolete alias (as of 27.1); use ‘cl-loop’ instead.
../p4/p4.el: Warning: ‘loop’ is an obsolete alias (as of 27.1); use ‘cl-loop’ instead.
../p4/p4.el: Warning: ‘loop’ is an obsolete alias (as of 27.1); use ‘cl-loop’ instead.
../p4/p4.el: Warning: ‘loop’ is an obsolete alias (as of 27.1); use ‘cl-loop’ instead.
../p4/p4.el: Warning: ‘loop’ is an obsolete alias (as of 27.1); use ‘cl-loop’ instead.
../p4/p4.el: Warning: ‘case’ is an obsolete alias (as of 27.1); use ‘cl-case’ instead.
../p4/p4.el: Warning: ‘loop’ is an obsolete alias (as of 27.1); use ‘cl-loop’ instead.
../p4/p4.el: Warning: ‘loop’ is an obsolete alias (as of 27.1); use ‘cl-loop’ instead.
../p4/p4.el: Warning: ‘pushnew’ is an obsolete alias (as of 27.1); use ‘cl-pushnew’ instead.
../p4/p4.el: Warning: ‘loop’ is an obsolete alias (as of 27.1); use ‘cl-loop’ instead.
../p4/p4.el: Warning: ‘loop’ is an obsolete alias (as of 27.1); use ‘cl-loop’ instead.
../p4/p4.el: Warning: ‘case’ is an obsolete alias (as of 27.1); use ‘cl-case’ instead.
../p4/p4.el: Warning: ‘loop’ is an obsolete alias (as of 27.1); use ‘cl-loop’ instead.
../p4/p4.el: Warning: ‘loop’ is an obsolete alias (as of 27.1); use ‘cl-loop’ instead.
../p4/p4.el: Warning: ‘case’ is an obsolete alias (as of 27.1); use ‘cl-case’ instead.
../p4/p4.el: Warning: ‘loop’ is an obsolete alias (as of 27.1); use ‘cl-loop’ instead.
../p4/p4.el: Warning: ‘loop’ is an obsolete alias (as of 27.1); use ‘cl-loop’ instead.
../p4/p4.el: Warning: ‘loop’ is an obsolete alias (as of 27.1); use ‘cl-loop’ instead.
../p4/p4.el: Warning: ‘incf’ is an obsolete alias (as of 27.1); use ‘cl-incf’ instead.
../p4/p4.el: Warning: ‘incf’ is an obsolete alias (as of 27.1); use ‘cl-incf’ instead.
../p4/p4.el: Warning: ‘incf’ is an obsolete alias (as of 27.1); use ‘cl-incf’ instead.
../p4/p4.el: Warning: ‘loop’ is an obsolete alias (as of 27.1); use ‘cl-loop’ instead.
../p4/p4.el: Warning: ‘incf’ is an obsolete alias (as of 27.1); use ‘cl-incf’ instead.
../p4/p4.el: Warning: ‘incf’ is an obsolete alias (as of 27.1); use ‘cl-incf’ instead.
../p4/p4.el: Warning: ‘incf’ is an obsolete alias (as of 27.1); use ‘cl-incf’ instead.
../p4/p4.el: Warning: ‘case’ is an obsolete alias (as of 27.1); use ‘cl-case’ instead.
../p4/p4.el: Warning: ‘loop’ is an obsolete alias (as of 27.1); use ‘cl-loop’ instead.
../p4/p4.el: Warning: ‘loop’ is an obsolete alias (as of 27.1); use ‘cl-loop’ instead.
../p4/p4.el: Warning: ‘loop’ is an obsolete alias (as of 27.1); use ‘cl-loop’ instead.
../p4/p4.el: Warning: ‘loop’ is an obsolete alias (as of 27.1); use ‘cl-loop’ instead.

[mathrick]

Sorry, I've been pretty busy since last week. I'm expecting to have some time to look at and properly understand your comments properly this weekend.

[mathrick]

I will squash the commits before merging this in, but I'm leaving them as-is for now because it makes it easier to ask questions about your changes.

[mathrick]

Is there a reason you've set the requirements to the absolutely latest and greatest version for magit and transient? I don't like aggressively recent requirements without a compelling argument, I find it unkind to the users to say that they must have the version that just got released if the code would work just fine with a much older requirement.

The versions I originally picked (3.3.0 and 0.4.3) roughly correspond to the oldest versions I have lying around on any of my machines and which I could test; is there anything wrong with those? If the code remains compatible with the latest versions while targetting old ones, I'd prefer to keep the oldest versions possible as the requirement.

[tarsius]

This is a major change and to me it makes sense to take the opportunity to make a clear cut.

Supporting very old versions has a very real cost, when done properly (i.e., not just claiming that old versions are supported when in fact it is unknown whether they are), and there are other areas where a maintainer's time is better invested. Stopping to support some old version is more difficult than to not support some old version from the get go. This is an existing package, but its a major upgrade so it is as close to "from the get to" as we can get here.

I do not know what the actual minimally required versions are. Figuring that out would be a massive time investment. I think it is unkind to the user to randomly pick some random version (e.g., "the oldest versions I have lying around"). They might end up sticking to what they have installed, assuming that the specified minimal versions are actually accurate, and then run into issues because the specified required versions were made up.

By requiring users to use a recent version, you force them to make the minimal time investment necessary to update. As an additional benefit they get a ton of fixed bugs (and sure, likely also some regressions).

Magit and Transient have many users, and supporting them takes a lot of time. I am barely able to keep up. So when someone reports a bug, I expect them to help me help them, and that involves updating to the latest releases. You would be surprised how often users report issues that have been fixed long ago. By "forcing" users to use a recent version up front, I save them from getting an initial response of "please update" when they report an issue, and then having to wait for me to get around to look at the report a second time.

Declaring that some older versions are supported, only helps users if the maintainer actually tests that this is still true (regularly, ideally automated). For my major packages I do that when it comes to the Emacs dependency. The cost of supporting old Emacs releases is large. It prevents the use of newer Emacs features and forces the use of workarounds for Emacs bugs that have been fixed long ago. I am not doing that for dependencies that are individual package, not even (or especially) my own packages.

Magit has many users but only one maintainer. I am just human not RHEL. If someone wants RHEL-like stability, they can pay my accordingly.

Magit 3.3.0 is 3.5 years old. It is not reasonable to still be using that. Users who get Emacs package with their distributions package manager, should question whether that is a reasonable thing to do, if it turns out that their distribution is stuck on such an old version.

Transient 0.4.3 has very serious issues. "Emacs accepts no input and has to be killed from the outside" kinda serious.

A reasonable compromise would be to require 4.0.0 and 0.8.0. If a user used these versions and reported an issue, I would still first ask them to update, but at least it is quite possibly safe to assume, that this package works with these versions.

You also have to bump the Emacs dependency. Magit 4.0.0 requires 26.1. Magit >= 4.2.0 requires 27.1. For that reason it is actually a good idea to only require 4.0.0, not 4.3.0, here for a while.