-
Notifications
You must be signed in to change notification settings - Fork 20
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
[fix] Modified functions using popen #15 #32 #48
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a search in the files of the project and I see two occurrences of io.popen()
that are not being changed, can you please double check?
See also my comment below.
output_file:write(command_file:read("*a")) | ||
command_file:close() | ||
output_file:seek('set',0) | ||
return output_file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't the file be removed once the output is read? And are you sure using tmp files is really necessary?
Wouldn't it be enough to just do:
output = commmand_file:read("*a")
command_file:close()
return output
We weren't doing this before explicitly for every call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a search in the files of the project and I see two occurrences of io.popen()
that are not being changed, can you please double check?
See also my comment below.
I did it intentionally. The reason being:
|
Hi @nemesisdesign , I updated the code to use split instead as we agreed in the chat. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's a lot of repetition in this PR, why not create a utility function which does the read, close and returning of the output?
This also ensures that whoever will put their hands on this code will keep reusing our function and avoid messing it up again by forgetting to close a pipe.
ba0c7a6
to
be0a5bf
Compare
I too tried doing something similar. But if I do it in utils function and import it in other sub-modules, then it may not be possible for now to keep them separated from one another. The cache property of |
I hope we can find a way to do it, it would be better, I created an issue here: #61. |
command_file:close() | ||
return output | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicating the same function in each file is actually worse Kapil. If duplicating two lines of code is not good, how can duplicating 6 lines of code be better? Please rollback how it was before.
be0a5bf
to
048db7b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
Fixes #15
Fixes #32