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

Re-add transient menu #132

Merged
merged 3 commits into from
Dec 8, 2024
Merged

Re-add transient menu #132

merged 3 commits into from
Dec 8, 2024

Conversation

deejayem
Copy link
Contributor

#121 added a transient menu, but this did not work with any currently released versions of Emacs (#129), so it was reverted.

I cherry-picked the two original commits, and added a third commit that fixes the issue (based on magit/transient#280 (comment)) by adding (autoload 'git-link-dispatch "git-link-transient" nil t) to the autoload line. I cherry-picked the commits as I didn't want to take credit for anyone else's work, but let me know if you want it done differently.

This fixes the issue for me on 29.4.

@sshaw
Copy link
Owner

sshaw commented Nov 23, 2024

Hi, thanks for this.

I didn't study #129 too closely but I assumed git-link-transient.el was somehow being loaded and transient was not defined but after seeing this PR looks like that is not true.

How did you reproduce the issue in #121 and have you tried this on versions of emacs that do not have transient?

@deejayem
Copy link
Contributor Author

Hi, thanks for this.

Thank you for creating git-link! I typically use it multiple times per week, and it saves me a lot of time.

I didn't study #129 too closely but I assumed git-link-transient.el was somehow being loaded and transient was not defined but after seeing this PR looks like that is not true.

How did you reproduce the issue in #121 and have you tried this on versions of emacs that do not have transient?

I was able to reproduce the issue on 29.x by installing the original version of git-link-transient.el and starting Emacs, giving this error:
⛔ Warning (emacs): Error loading "git-link" autoloads: (void-function transient-prefix)
After the change in my extra commit, 29.x starts without error, and git-link-dispatch works.

On 27.x, I see the same behaviour in terms of the error at startup. The difference is that if transient isn't installed as a dependency of something else, when I run git-link-dispatch I get the error Cannot open load file: No such file or directory, transient. (Transient was added to Emacs in 28.1).

Based on this comment in git-link-transient.el I think that's the intended behaviour:

;; You need to have `transient' installed as a dependency.
;; (it's not listed as the dependency of git-link because we want it to be optional.)

I think it would be better to depend on transient rather than it being broken, but it's up to you. Anyone who has magit installed will already have transient, as will anyone on 28.1+.

(By the way, unless you're using something like straight or elpaca, the easiest way to test it is to copy git-link-transient.el into the git-link directory in ~/.emacs.d/elpa/ and then run (package-generate-autoloads "git-link" "~/.emacs.d/elpa/git-link-20241022.2208/")).

@deejayem
Copy link
Contributor Author

@sshaw Do you have any thoughts on how you would like to handle this?

I can think of three possibilities:

  1. Use the approach already taken on this PR. This means that with 28.1+, everything will work fine. For older versions, there will be no errors at startup (unlike with the original version), but git-link-dispatch will give an error unless transient is already installed. (I'm not keen on that, but if people also use magit they won't have an issue).

  2. Add transient to Package-Requires. This would require increasing the minimum supported Emacs version to 26.1.

  3. Create a separate git-link-transient package. Either you or @blahgeek could do that, but I can do it if neither of you want to.

There may be some other approach that I haven't thought of though.

@sshaw
Copy link
Owner

sshaw commented Dec 4, 2024

Hi,

Thanks for all the info!

  1. Use the approach already taken on this PR. This means that with 28.1+, everything will work fine. For older versions, there will be no errors at startup (unlike with the original version), but git-link-dispatch will give an error unless transient is already installed. (I'm not keen on that, but if people also use magit they won't have an issue).

git-link-dispatch would only be called if they wanted to use the transient menu, right? If so I think this is okay and is sorta typical Emacs behavior. As long as non git-link transient users are not affected.

Add transient to Package-Requires. This would require increasing the minimum supported Emacs version to 26.1.

I try to keep dependencies at a minimum though supporting 26 in 2024/25 does not seem too crazy. But I think point 1 is better approach. Though why 26 if transient was adding added in 28?

@deejayem
Copy link
Contributor Author

deejayem commented Dec 5, 2024

Hi,

Thanks for all the info!

  1. Use the approach already taken on this PR. This means that with 28.1+, everything will work fine. For older versions, there will be no errors at startup (unlike with the original version), but git-link-dispatch will give an error unless transient is already installed. (I'm not keen on that, but if people also use magit they won't have an issue).

git-link-dispatch would only be called if they wanted to use the transient menu, right? If so I think this is okay and is sorta typical Emacs behavior. As long as non git-link transient users are not affected.

Correct. Unless people explicitly try to use git-link-dispatch, they won't notice any difference. If you're happy with this approach, this PR should be good to merge now.

Add transient to Package-Requires. This would require increasing the minimum supported Emacs version to 26.1.

I try to keep dependencies at a minimum though supporting 26 in 2024/25 does not seem too crazy. But I think point 1 is better approach. Though why 26 if transient was adding added in 28?

Transient only supports 26+, so on 26 and 27 it can be installed from ELPA. I haven't tested what happens if you try to use it on older versions though.

@sshaw
Copy link
Owner

sshaw commented Dec 8, 2024

Great thanks! Let's give it a whirl!

@sshaw sshaw merged commit 40f7b16 into sshaw:master Dec 8, 2024
9 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