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

Cli: Fixes #5341: Ignore newline between quotes while spliting batch #5540

Merged
merged 7 commits into from
Oct 16, 2021
Merged

Cli: Fixes #5341: Ignore newline between quotes while spliting batch #5540

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ab1930b
Cli: Fixes #5341: Ignore newline between quotes while spliting batch …
kkoyung Oct 7, 2021
61efbd1
separate splitting process to lib/string-utils.js, and add a test for it
kkoyung Oct 9, 2021
45e5079
Merge branch 'laurent22:dev' into fix-multi-line-batch
kkoyung Oct 13, 2021
071862f
update test cases for `splitCommandBatch()`
kkoyung Oct 13, 2021
4b57792
change to use finite state machine in splitCommandBatch(), with an ex…
kkoyung Oct 16, 2021
2f88f5b
minor changes on splitCommandBatch() and its corresponding test
kkoyung Oct 16, 2021
4afb192
Merge branch 'laurent22:dev' into fix-multi-line-batch
kkoyung Oct 16, 2021
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
5 changes: 3 additions & 2 deletions packages/app-cli/app/app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const Tag = require('@joplin/lib/models/Tag').default;
const Setting = require('@joplin/lib/models/Setting').default;
const { reg } = require('@joplin/lib/registry.js');
const { fileExtension } = require('@joplin/lib/path-utils');
const { splitCommandString } = require('@joplin/lib/string-utils');
const { splitCommandString, splitCommandBatch } = require('@joplin/lib/string-utils');
const { _ } = require('@joplin/lib/locale');
const fs = require('fs-extra');
const { cliUtils } = require('./cli-utils.js');
Expand Down Expand Up @@ -390,7 +390,8 @@ class Application extends BaseApplication {
async commandList(argv) {
if (argv.length && argv[0] === 'batch') {
const commands = [];
const commandLines = (await fs.readFile(argv[1], 'utf-8')).split('\n');
const commandLines = splitCommandBatch(await fs.readFile(argv[1], 'utf-8'));

for (const commandLine of commandLines) {
if (!commandLine.trim()) continue;
const splitted = splitCommandString(commandLine.trim());
Expand Down
23 changes: 23 additions & 0 deletions packages/lib/StringUtils.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
/* eslint-disable no-unused-vars */

const { splitCommandBatch } = require('./string-utils');
const StringUtils = require('./string-utils');
const { EOL } = require('os');

describe('StringUtils', function() {

Expand Down Expand Up @@ -53,4 +55,25 @@ describe('StringUtils', function() {
});
}));

it('should split the command batch by newlines not inside quotes', (async () => {
const testCases = [
['',
['']],
['command1',
['command1']],
['command1 arg1 arg2 arg3',
['command1 arg1 arg2 arg3']],
[`command1 arg1 'arg2${EOL}continue' arg3`,
[`command1 arg1 'arg2${EOL}continue' arg3`]],
[`command1 arg1 'arg2${EOL}continue'${EOL}command2${EOL}command3 'arg1${EOL}continue${EOL}continue' arg2 arg3`,
[`command1 arg1 'arg2${EOL}continue'`, 'command2', `command3 'arg1${EOL}continue${EOL}continue' arg2 arg3`]],
[`command1 arg\\1 'arg2${EOL}continue\\'continue' arg3`,
[`command1 arg\\1 'arg2${EOL}continue\\'continue' arg3`]],
];
Copy link
Owner

Choose a reason for hiding this comment

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

You split on EOL, which is OS specific, but your code only handle "\n". I'm fine if we only support "\n", but the tests should reflect that. So please set your own variable const eol = "\n" and use that.


testCases.forEach((t) => {
expect(splitCommandBatch(t[0])).toEqual(t[1]);
});
}));
laurent22 marked this conversation as resolved.
Show resolved Hide resolved

});
54 changes: 53 additions & 1 deletion packages/lib/string-utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,58 @@ function splitCommandString(command, options = null) {
return args;
}

function splitCommandBatch(commandBatch) {
const commandLines = [];

let state = 'command';
let current = '';
let quote = '';
for (let i = 0; i < commandBatch.length; i++) {
const c = commandBatch[i];

if (state == 'command') {
if (c == '\n') {
commandLines.push(current);
current = '';
} else if (c == '"' || c == '\'') {
quote = c;
current += c;
state = 'quoted';
} else if (c == '\\') {
current += c;
if (i + 1 < commandBatch.length) {
current += commandBatch[i + 1];
i++;
}
} else {
current += c;
}
} else if (state == 'quoted') {
if (c == quote) {
quote = '';
current += c;
state = 'command';
} else if (c == '\\') {
current += c;
if (i + 1 < commandBatch.length) {
current += commandBatch[i + 1];
i++;
}
} else {
current += c;
}
}
}
if (current.length > 0) {
commandLines.push(current);
}
if (commandLines.length == 0) {
commandLines.push('');
}

return commandLines;
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the update. In your function please use strict equality everywhere (=== and !==). All new code should use this.

}
laurent22 marked this conversation as resolved.
Show resolved Hide resolved

function padLeft(string, length, padString) {
if (!string) return '';

Expand Down Expand Up @@ -307,4 +359,4 @@ function scriptType(s) {
return 'en';
}

module.exports = Object.assign({ formatCssSize, camelCaseToDash, removeDiacritics, substrWithEllipsis, nextWhitespaceIndex, escapeFilename, wrap, splitCommandString, padLeft, toTitleCase, urlDecode, escapeHtml, surroundKeywords, scriptType, commandArgumentsToString }, stringUtilsCommon);
module.exports = Object.assign({ formatCssSize, camelCaseToDash, removeDiacritics, substrWithEllipsis, nextWhitespaceIndex, escapeFilename, wrap, splitCommandString, splitCommandBatch, padLeft, toTitleCase, urlDecode, escapeHtml, surroundKeywords, scriptType, commandArgumentsToString }, stringUtilsCommon);