[fix] Modified functions using popen #15 #32

[devkapilbansal]

Fixes #15
Fixes #32

[nemesifier]

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.

[nemesifier]

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.

[nemesifier]

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.

[devkapilbansal]

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:

• The utils.popen was getting cached in some cases which results in breaking the tests. The solution can be to mock define utils.popen too in each file which I don't think is a cleaner solution.
• We need to modify popen only when we are performing operations before closing the file. The only scenario in our case was looping over the output using :lines() function.

[devkapilbansal]

Hi @nemesisdesign , I updated the code to use split instead as we agreed in the chat.

[nemesifier]

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.

[devkapilbansal]

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.

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 require is breaking the functionalities in tests 😕

[nemesifier]

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.

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 require is breaking the functionalities in tests

I hope we can find a way to do it, it would be better, I created an issue here: #61.

[nemesifier]

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.

[nemesifier]

Thanks