Fix quoting of string columns in csv #1379
Conversation
If the `#select` resultset doesn't exist, arbitrarily choose the first result set when viewing csv results. This will almost certainly be the correct result set. In the future, we could offer a popup if there are multiple result sets available, but let's wait on that until someone actually asks for it.
aeisenberg
added
the
Complexity: Low
1. `exportCsvResults` now no longer requires an `onFinish` callback. 2. The test adds a generic framework for creating a mock cli server. This should be used in future tests.
|
Adding @edoardopirovano as a reviewer since he is already familiar with this area. |
| const quotes = chunk.columns.map(col => col.kind === 'String' ? '"' : ''); | ||
| chunk.tuples.forEach((tuple) => { | ||
| out.write(tuple.map((v, i) => { | ||
| return `${quotes[i]}${v}${quotes[i]}`; |
There was a problem hiding this comment.
I think we also need to be escaping any " characters in v when the column is a String (maybe it's safe to do this unconditionally if the other column types can't contain quote characters?). I believe this ought to be done by double quoting, but do check what the prevailing CSV convention is rather than taking my word for it 🙂
There was a problem hiding this comment.
Good point. I'll do that.
| void this.tryOpenExternalFile( | ||
| query.csvPath | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Should we be displaying an error in the else case here? I see we reject() where we were previously using showAndLogErrorMessage() but as far as I can tell we are then swallowing that error.
There was a problem hiding this comment.
@aeisenberg I don't think you've addressed this comment. It's less blocking the other others, so I'm happy to approve without any changes here if you don't think any changes are needed, but just checking it wasn't missed accidentally.
There was a problem hiding this comment.
Oh...sorry. I forgot to comment here. The error is not being swallowed. Take a look at commandRunnerWithProgress, you can see that there is an error barrier there that catches and logs all thrown and rejected errors. So, you don't ever need to explicitly catch and log an error when you're inside a command as long as that command is wrapped in commandRunnerWithProgress.
There was a problem hiding this comment.
And, indeed, when I try this out, forcing an error inside of exportCsvResults, the error is logged appropriately.
There was a problem hiding this comment.
Gotcha, thanks for the explanation 🙂
| bqrsInfo: [{ 'result-sets': [{ name: resultSetName }, { name: 'hucairz' }] }], | ||
| bqrsDecode: [{ | ||
| columns: [{ kind: 'NotString' }, { kind: 'String' }], | ||
| tuples: [['a', 'b'], ['c', 'd']], |
There was a problem hiding this comment.
Let's expand the tuples here to include some quote characters and make sure those get escaped correctly.
|
@edoardopirovano can I get another look? |
Fixes two issues with CSV exporting and creates a unit test along with a generic framework for creating a mock CLI server. Commit-by-commit review is suggested.
Fixes #1377
Fixes #1376
Checklist
ready-for-doc-reviewlabel there.