Fix issue #11

[moos]

Tested on OSX and CentOS & old tests pass on Windows.

[neekey]

Thanks for your contribution! Seems like a interesting solution, will dive deep when i come back, i'm on my vacation in Thailand this week ; )

On Tue, 2 May 2017 at 07:12 Moos ***@***.***> wrote: Tested on OSX and CentOS & old tests pass on Windows. ------------------------------ You can view, comment on, or merge this pull request online at: #56 Commit Summary - Fix for issue #11 - path containing spaces. - Fix for issue #11 - path containing spaces. File Changes - *M* lib/index.js <https://github.com/neekey/ps/pull/56/files#diff-0> (32) - *M* test/test.js <https://github.com/neekey/ps/pull/56/files#diff-1> (24) - *A* test/with space/sleep with space <https://github.com/neekey/ps/pull/56/files#diff-2> (1) Patch Links: - https://github.com/neekey/ps/pull/56.patch - https://github.com/neekey/ps/pull/56.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#56>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAegnkCkmQVfrv401ktWH-BD6KfTOeLOks5r1nUKgaJpZM4NNk4E> .

-- 发自移动版 Gmail

[moos]

ping - have you had a chance to look at this yet?

[neekey]

it's not a good practice to change the parameter cmd and even use it later, too implicit. I would suggest you write a command parser like:

function commandParser(cmd) { ... }

var ret = commandParser(cmd);
var command = ret.command;
var args = ret.args; 

[neekey]

Still, I would not change the cmd directly, I would either get a copy of cmd or just use an index to do the shifting.

[neekey]

Sorry for the late reply : )

I think the file existence checking is a brilliant idea, but my concern is that it may create performance issue if there's a huge number of results returned and you will have to check every command segment, which will end up with a lot of IO workload.

Since I don't have a better solution for this issue, I think there are two things that can do to make the situation better for most of the users:

• the issue is actually an edge case, most of the users may not even notice this issue, so I will suggest making this solution as an optional feature, we can create an option for the user to toggle it
• for the performance issue, you can actually cache the exists check result by saving a key-value object with a key as the command segment and the value as a boolean to indicate the command segment exists or not, it will be helpful if the result contains a lot of same commands or the module is run by multiple times

[moos]

No problem. It's an edge case if you don't run into it :). Working on Google Chrome I ran into this very early. So well anyone else working with paths that have spaces.

As for performance, I was concerned too so I ran a rudimentary test and noticed no difference. This fixes a fundamental flaw in the module -- I really don't think making it optional is the right path.

I've recently published npm/chromate which fails due to this issue.