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

Add support for executing recipes in vterm #55

Merged
merged 5 commits into from
Jul 1, 2024
Merged

Conversation

skissue
Copy link
Contributor

@skissue skissue commented Jun 21, 2024

As proposed in #54.

I just went ahead and chose v and V as the keybinds since they made sense to me, but I'm open to changing it if desired. RIght now the keys are always bound and then vterm's presence is checked when the commands are called (via require); I'm not sure if this is the best way to support this as an "optional" feature, so I'm open to feedback on that as well.

Copy link
Owner

@psibi psibi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about introducing a new defcustom instead ? Something like justl-default-shell with two choices: eshell and vterm. eshell can be the default.

The advantage that is that there is no need to introduce new keybinding. Based on the option configured, you use the existing keybinding.

@skissue
Copy link
Contributor Author

skissue commented Jun 22, 2024

I went ahead and changed it to use the justl-shell option, which defaults to eshell, and converted justl-exec-eshell into a generic justl-exec-shell function. I'm not sure if the code is the cleanest it could be 😅, but everything seems to be working for me.

Additionally, I'm wondering whether an obsolete alias should be added for justl-exec-eshell, since the function has been renamed 🤔.

Copy link
Owner

@psibi psibi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I liked your previous implementation even though it will likely add more code. Can you do the following:

  • Define new function justl-exec-vterm and justl-no-exec-vterm similar to how you did before.
  • Create new function justl-exec-shell and justl-no-exec-shell as you have defined in the latest commit. This function should call the apprpriate functions based on the justl-default-shell variable.

The main motivation is to make it easier to add support for other shells.

@skissue
Copy link
Contributor Author

skissue commented Jun 24, 2024

  • Define new function justl-exec-vterm and justl-no-exec-vterm similar to how you did before.

Done.

  • Create new function justl-exec-shell and justl-no-exec-shell as you have defined in the latest commit. This function should call the apprpriate functions based on the justl-default-shell variable.

Done. I updated the default keybinds to use these functions.

The main motivation is to make it easier to add support for other shells.

I agree that this is a better approach to accomplish this goal.

Let me know if there are any other changes you'd like me to make.

@psibi psibi merged commit e74fef3 into psibi:master Jul 1, 2024
5 checks passed
@psibi
Copy link
Owner

psibi commented Jul 1, 2024

Thank you!

@skissue skissue deleted the vterm branch July 1, 2024 13:14
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.

2 participants