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

[Console] Move out of quarantined #52270

Merged
merged 22 commits into from
Dec 10, 2019
Merged

[Console] Move out of quarantined #52270

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
f53964c
WiP, lotta broken things, working through new editor.ts
jloleysens Nov 25, 2019
ba78edd
RowParser -> TS
jloleysens Nov 29, 2019
d92621c
Utils to TS and regular module
jloleysens Nov 29, 2019
05bb02b
Finished first version of core & sense editor wrappers. Tokenizer pro…
jloleysens Dec 2, 2019
18d0093
WiP - moved A LOT of code around and busy fixing sense-editor tests
jloleysens Dec 3, 2019
0f2ef09
Fix sense editor test
jloleysens Dec 3, 2019
458cd57
Clean up mocks
jloleysens Dec 3, 2019
78bbd57
Moved A LOT of code out of quarantined.
jloleysens Dec 4, 2019
47f5191
WiP still finishing up manual testing
jloleysens Dec 4, 2019
f733e8f
Fix use of Ace Range and fix open documentation
jloleysens Dec 4, 2019
6feb90c
Move out of quarantined!
jloleysens Dec 5, 2019
3d0259f
Remove load remote editor state for now
jloleysens Dec 5, 2019
dd2f3e1
- fix use of token iterator
jloleysens Dec 5, 2019
c7157e7
Address getSelectionRange document TODO
jloleysens Dec 6, 2019
59ea7d0
Update src/legacy/core_plugins/console/public/np_ready/application/mo…
jloleysens Dec 6, 2019
62cec7c
Remove commented-out code
jloleysens Dec 9, 2019
6cff57b
Rename format request function
jloleysens Dec 9, 2019
59cc081
utils.ts default export usage cleanup
jloleysens Dec 9, 2019
f3b80b9
Merge branch 'master' into console/sense-editor-core-editor
elasticmachine Dec 9, 2019
a0e39bb
Update replace regex and add another utils test
jloleysens Dec 9, 2019
4441976
Remove legacy replace behaviour
jloleysens Dec 10, 2019
d6bc4b7
Fix typo in comment
jloleysens Dec 10, 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
12 changes: 12 additions & 0 deletions ...egacy/core_plugins/console/public/np_ready/lib/utils/__tests__/utils_string_expanding.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,3 +40,15 @@ Correctly parse with JSON embedded inside values
"content\\\\": """ { "json": "inside\\" ok! 1
" } """
}
==========
Correctly handle new lines in triple quotes
-------------------------------------
{
"query": "\n SELECT * FROM \"TABLE\"\n "
}
-------------------------------------
{
"query": """
SELECT * FROM "TABLE"
"""
}
4 changes: 2 additions & 2 deletions src/legacy/core_plugins/console/public/np_ready/lib/utils/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,8 @@ export function expandLiteralStrings(data: string) {
const colonAndAnySpacing = string.slice(0, firstDoubleQuoteIdx);
const rawStringifiedValue = string.slice(firstDoubleQuoteIdx, string.length);
const jsonValue = JSON.parse(rawStringifiedValue)
.replace('^s*\n', '')
.replace('\ns*^', '');
.replace('^\s*\n', '') // prettier-ignore
.replace('\n\s*$', ''); // prettier-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Prettier is correct here - these patterns be surrounded in forward-slashes.

      const jsonValue = JSON.parse(rawStringifiedValue)
        .replace(/^\s*\n/, '')
        .replace(/\n\s*$/, '');

I see what you mean about the difference with trim() - it would remove all whitespace at the start or end of string, whereas these patterns are a little more subtle. Maybe it would be worth a comment, in case someone comes along later and replaces it with trim(). 😀

Copy link
Contributor

Choose a reason for hiding this comment

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

(Although I still query the usefulness of this vs. a simple trim())

Copy link
Contributor Author

@jloleysens jloleysens Dec 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Come to think of it, this replace behaviour was part of legacy and was carried over from a previous alteration. I'm not convinced we want replace behaviour here at all. The value being altered was inside of """ which means string literal in Console. Newlines and whitespace should be preserved. I don't think those replace functions where actually doing anything when the behaviour was considered "correct":

"\n    SELECT * FROM \"TABLE\"\n    "
        .replace('^\s*\n', '')
        .replace('\n\s*$', '')

===

"\n    SELECT * FROM \"TABLE\"\n    "

// true

Automated and manual testing seem to confirm this. Would you mind taking a look too @pugnascotia ?

return `${colonAndAnySpacing}"""${jsonValue}"""`;
} else {
return string;
Expand Down