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.

Sign up for GitHub

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

fixed handling of quoted commands with spaces #21

Merged
merged 5 commits into from
Nov 13, 2019
Merged

fixed handling of quoted commands with spaces #21

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ca09b8e
fixed handling of quoted commands with spaces
omjadas Oct 12, 2019
30e48ea
documented parameter regex
omjadas Oct 14, 2019
9acb670
always set shell to true when spawning
omjadas Nov 3, 2019
b2d4ddc
removed spawnOptions
omjadas Nov 8, 2019
617706e
join array instead of using manual spaces
omjadas Nov 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/Process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ export const createProcess = (
// as they can only be the first command, so take out the command, and add
// any other bits into the args
const runtimeExecutable = workspace.pathToJest;
const parameters = runtimeExecutable.split(' ');
const parameters = runtimeExecutable.match(
/(?:"[^"\\]*(?:\\[\S\s][^"\\]*)*"|'[^'\\]*(?:\\[\S\s][^'\\]*)*'|(?:\\\s|\S))+/g
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I get what you are trying to do, but it is a bit complex for the mere mortal: nested non-capture-groups, [\S\s] (isn't white-space + non-white-space = well, everything?)... I was having a hard time following, wondering if it is possible to simplify it, came up with this

/(\S*".*?")|(\S*'.*?')|(\S*\\\s\S+)|\S+/g

there are really just 3 patterns we want to capture beyond the non-white-space: single, double quotes and backslash escape... tested with the jest and the following, both seem to work:

"/a/b is a directory/c" --without /x/y/z useThis="I am a 'nested single quote' test" single='this is a single quote'  backslash=a\\ space/b/c

however, the nested quotes cause problems for both regex patterns:

--nestedQuote="before \\"after this\\" end" --nestedSingleQuote='before \\'after this\\' end'

while this can be easily fixed with lookbehind (I am sure you can also fix your regex pattern), it does start to get more complicated and ugly... I can be talked into not to support it as I don't see how this will occur in jest command line...

/(\S*".*?(?<!\\)")|(\S*'.*?(?<!\\)')|(\S*\\\s\S+)|\S+/g

anyway, just to throw some idea out there to see if we can get it simplified a bit...

BTW, to make future maintenance easier, how about adding some comments to explain the regex patterns?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for [\S\s] is that it will match newlines, whereas . will not. This is in case of something like this

"/a/b is a directory/c" --without /x/y/z useThis="I am a 'nested 
single quote' test" single='this is a single quote'  backslash=a\\ space/b/c

Also, I'm pretty sure that

--nestedQuote="before \\"after this\\" end"

should be two matches. Whereas

--nestedQuote="before \"after this\" end"

should be one match.

I noticed that the regex you suggested split

"/a/b is\" adirectory/c"

into two matches. It's unlikely that someone would have a quote in their path, but it is possible.

Not sure how to simplify it and still support these cases.

) || [''];
const command = parameters[0];
const initialArgs = parameters.slice(1);
const runtimeArgs = [].concat(initialArgs, args);
Expand Down
6 changes: 3 additions & 3 deletions src/__tests__/process.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,15 @@ describe('createProcess', () => {
expect(spawn.mock.calls[0][1]).toEqual(['test', '--']);
});

it('fails to spawn the first quoted arg from workspace.pathToJest', () => {
it('spawns the first quoted arg from workspace.pathToJest', () => {
const workspace: any = {
pathToJest: '"../build scripts/test" --coverageDirectory="../code coverage"',
};
const args = [];
createProcess(workspace, args);

expect(spawn.mock.calls[0][0]).not.toBe('"../build scripts/test"');
expect(spawn.mock.calls[0][1]).not.toEqual(['--coverageDirectory="../code coverage"']);
expect(spawn.mock.calls[0][0]).toBe('"../build scripts/test"');
expect(spawn.mock.calls[0][1]).toEqual(['--coverageDirectory="../code coverage"']);
});

it('appends args', () => {
Expand Down