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

Feature/interactive aliases #154

Merged

Conversation

ZappaBoy
Copy link
Contributor

Concerning #152 I added the functionality of executing commands via interactive shell.
I tried to be as minimally invasive as possible by making few changes and respecting the programming style.

By default the feature is off but by setting the "interactive_shell" property to "True" in preferences the commands are executed based on the shell set ($SHELL env)

This is only a first step as processes launched in the background are not shown to the user but I am working on it in my spare time.

Anyway I am submitting these changes since I don't know when I can work on them due to my jobs.

Also finished this feature I would like to do a minimum of refactoring always free time permitting.

@ZappaBoy
Copy link
Contributor Author

Hi everybody, I wish to know if there are some problems with this PR or it can be merged?

@MarkHedleyJones MarkHedleyJones self-requested a review November 5, 2022 09:00
Copy link
Owner

@MarkHedleyJones MarkHedleyJones left a comment

Choose a reason for hiding this comment

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

Hi @ZappaBoy,
Thanks so much for creating a PR for your feature request 👍
I tested it and it works well.
I left a couple of comments that I'd like you to address, and then I'm happy to merge it into the main branch.

@@ -750,7 +751,10 @@ def execute(self, command, fork=None):
print("Command converted into:")
print(command)

return subprocess.call(command)
if self.prefs["interactive_shell"] is True:
shell = os.environ.get("SHELL", "/bin/bash")
Copy link
Owner

Choose a reason for hiding this comment

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

Rather than "/bin/bash" how about "/usr/bin/env bash" to handle compatibility with systems that don't have bash under the /bin directory.

Copy link
Contributor Author

@ZappaBoy ZappaBoy Nov 5, 2022

Choose a reason for hiding this comment

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

I perfectly agree with you. Changed in 0a30a44 .

@@ -203,6 +203,7 @@ def unsatisfied_plugin_requirements(plugin):
"indicator_edit": "*", # Symbol to indicate an item will launch an editor
"indicator_alias": "", # Symbol to indicate an aliased command
"prompt": "Open:", # Prompt
"interactive_shell": False, # Run commands in interactive mode
Copy link
Owner

Choose a reason for hiding this comment

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

For the comment, how about "Run commands in an interactive shell session".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 0a30a44 .

Moved interactive shell to "/usr/bin/env bash" to improve compatibility;
Referred to MarkHedleyJones#154 #discussion_r1014608222.
@ZappaBoy
Copy link
Contributor Author

ZappaBoy commented Nov 9, 2022

I also increased the version string, now the CI pipeline will run successfully.

@MarkHedleyJones MarkHedleyJones self-requested a review November 20, 2022 20:44
Add acknowledgement
Copy link
Owner

@MarkHedleyJones MarkHedleyJones left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Thanks very much for your patience and time taken to implement this improvement.
I have added your name to the list of Contributions 😄

@MarkHedleyJones MarkHedleyJones merged commit 3cd31bc into MarkHedleyJones:main Nov 20, 2022
@ZappaBoy ZappaBoy deleted the feature/interactive-aliases branch November 22, 2022 13:04
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