Skip to content

Fix getExecCommand() to get the latest command set instead only the first… #55

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

Merged
merged 2 commits into from
Mar 17, 2021

Conversation

hAbd0u
Copy link
Contributor

@hAbd0u hAbd0u commented Mar 13, 2021

Currently getExecCommand() outputs only the first command get executed, even if we set a new command via setCommand(), a work around it was to reinitiate the Command class each time when setting a new command, but with this pull request the issue will be solved.

@mikehaertl
Copy link
Owner

a work around it was to reinitiate the Command class each time when setting a new command

But that is the whole point: This class really only represents one execution! Afterwards you can fetch the resulting output and error. There is no reset of this information. It's also not clear what should happen to arguments you have set: preserve them? reset them?

So whenever you want to execute a new command, you should really create a fresh new instance.

@hAbd0u
Copy link
Contributor Author

hAbd0u commented Mar 13, 2021

It seems your vision is different from the name of the repository, basically when you execute a command in shell and later decide to execute another one then you are not obliged to lunch a new shell, you just use current.

@mikehaertl
Copy link
Owner

It's shell command - not shell. So one command refers to one instance. But your change basically only removes the internal caching of the executed command string. I think, it doesn't hurt if we change that. It wasn't really neccessary in the first place.

@mikehaertl mikehaertl merged commit 3488d78 into mikehaertl:master Mar 17, 2021
@mikehaertl
Copy link
Owner

@hAbd0u Just released 1.6.4 containing this change. Thanks for your contribution!

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