CSV connector #375
CSV connector #375
Conversation
n-riesco
commented
Feb 15, 2018
n-riesco
commented
- Do not merge before test: replace chai-spies with sinon #374
n-riesco
force-pushed
the
csv-connector
branch
from
1698f97 to
dc00d62
Compare
backend/persistent/datastores/csv.js
Outdated
| const connectionData = {}; | ||
|
|
||
| // define type error thrown by this connector | ||
| export function CSVError(url, errors) { |
There was a problem hiding this comment.
You created your own CSV Error with specific information. I am just trying to clarify why you needed your own error type. How will this be handled on the client?
|
@n-riesco I did a review this and everything looks good with the exception of creating your own error. I just wanted to better understand this and how these errors will be displayed to the client. |
|
@shannonlal At the moment, the frontend only shows The reason I defined a new error type was because I don't know yet if I'm going to stick to The PR is still work in progress. It works, but it still has a few rough corners. |
* This connector takes a URL as input, downloads the URL and parses it
as a CSV file into a Javascript object that can be queried using
alaSQL.
* Added test specs and the script `test-unit-csv` to run them.
* Updated `Setting.react.js` so that future connectors don't need to
update it (updated `NEW_CONNECTION.md` accordingly).
* TODO (in future PRs):
- `papaparse` parses CSV data only into numbers and strings. Add
option to let the user set a date format for parsing.
- let the user assign a table name
- let the user upload multiple CSV files
Closes plotly#349
n-riesco
force-pushed
the
csv-connector
branch
from
dc00d62 to
aecf162
Compare
|
@shannonlal I've just fixed the last few rough corners with this PR. I think the PR is ready for review. Would you like to review it? |
|
@n-riesco Yes I will have time to look this over on the weekend |
backend/persistent/datastores/csv.js
Outdated
| if (cell) return type(cell); | ||
| } | ||
|
|
||
| return 'String'; |
There was a problem hiding this comment.
Maybe just add a quick comment to explain that default is string. Or add to JSDocs to function
backend/persistent/datastores/csv.js
Outdated
| CSVError.prototype.constructor = CSVError; | ||
|
|
||
| // the data parsed from CSV files is stored here | ||
| const connectionData = {}; |
There was a problem hiding this comment.
In #362 One of the comments was to start using JSDocs style for our code. Should we be doing this on the backend as well?
| connectionData[connection.database] = data; | ||
| } | ||
|
|
||
| export function connect(connection) { |
There was a problem hiding this comment.
If the url is bad, how will this be presented to the user on the Settings UI? will fetch throw an error and this will be gracefully passed to the UI on its own?
There was a problem hiding this comment.
Same behaviour for all the connectors, i.e.: fetch throws, routes.js catches the exception and replies to the connection request with a 500 status and the body {error: message: error.message}}.
backend/persistent/datastores/csv.js
Outdated
|
|
||
| export function tables() { | ||
| // To take advantage of alaSQL's parser, the table is named '?' | ||
| return Promise.resolve(['?']); |
There was a problem hiding this comment.
Should we be using '?' or a const CSV_TABLE_NAME = '?'. This way if it changes in the future you don't have to string replace to make sure we find all changes
n-riesco
force-pushed
the
csv-connector
branch
from
89ddd97 to
92fc235
Compare
