DB Connector for data.world by kndungu · Pull Request #332 · plotly/falcon

kndungu · 2018-01-02T15:06:08Z

No description provided.

n-riesco · 2018-01-03T09:57:23Z

Thank you for this PR!
I'll review it later today.

kndungu · 2018-01-03T10:02:45Z

Sure thing! 👍

kndungu · 2018-01-08T14:51:06Z

@n-riesco

I've been able to mock data.world API responses so the the PR now includes tests. Looking forward to your feedback.

n-riesco · 2018-01-08T16:19:07Z

+        {
+            'label': 'Token',
+            'value': 'token',
+            'type': 'text',


'type': 'password'

n-riesco · 2018-01-08T16:26:19Z

+            'description': 'The URL of the dataset or project on data.world'
+        },
+        {
+            'label': 'Token',


data.world provides two API tokens: Read/Write and Admin.

To help users, we could re-label this input as Read/Write API Token.

n-riesco · 2018-01-08T16:40:04Z

+    ], // TODO - password options for apache drill
+    [DIALECTS.DATA_WORLD]: [
+        {
+            'label': 'Dataset/Project URL',


I'd like to change this. Instead of having the user to input an URL and the connector to parse the URL to determine owner and dataset id. I'd like the user to input owner and id:

this is more consistent with the existing connectors

and it simplifies the code (no need for getPathNames)

Hi, @n-riesco, unless you feel strongly about this, we would like to use the URL. That would be consistent with our other connectors work (e.g. Tableau and Google Data Studio) and is more usable, as users only have one value to copy and paste, which is readily available on the browser for the dataset they are looking at. We do a little bit of extra work in code to spare the user in real life. Also, using URIs that contain information to parse out isn't without a precedent, the most common is JDBC.

No, I don't feel strongly about it.

@tarzzz @chriddyp are you OK with using an URL here?

is more usable, as users only have one value to copy and paste, which is readily available on the browser for the dataset they are looking at

This sounds like a good reason to me 👍

n-riesco · 2018-01-08T16:47:36Z

+
+export function connect(connection) {
+  const { owner, id } = parseUrl(connection.url);
+  return new Promise((resolve, reject) => {


No need to create a new Promise, just return fetch(... and replace reject(json) by throw new Error(JSON.stringify(json)).

n-riesco · 2018-01-08T16:49:08Z

+ }
+
+export function tables(connection) {
+  return new Promise((resolve, reject) => {


No need to create a new Promise, just return query(...

n-riesco · 2018-01-08T16:49:38Z

+      resolve(allTables);
+    })
+    .catch(err => {
+      reject(err);


Logger.log(err); throw err;

n-riesco · 2018-01-08T16:50:16Z

+    })
+    .catch(err => {
+      Logger.log(err);
+      throw new Error(err);


throw err

n-riesco · 2018-01-08T16:54:00Z

+}
+
+export function schemas(connection) {
+  return new Promise((resolve) => {


No need to create a new Promise, just return query(... and replace resolve({...}) by return {...}.

n-riesco · 2018-01-08T16:54:17Z

+    })
+    .catch(err => {
+      Logger.log(err);
+      throw new Error(err);


throw err

n-riesco · 2018-01-08T17:00:04Z

+export function query(queryString, connection) {
+  const { owner, id } = parseUrl(connection.url);
+  return new Promise((resolve, reject) => {
+    const queryStatement = `${queryString.replace(/-/g, '_')}`;


This substitution could potentially break the SQL query,

I'd move this code (that replaces - with _) into tables and schemas to ensure the user only sees table names without -.

n-riesco · 2018-01-08T17:00:48Z

+
+export function query(queryString, connection) {
+  const { owner, id } = parseUrl(connection.url);
+  return new Promise((resolve, reject) => {


No need to create a new Promise, just return fetch(...

n-riesco · 2018-01-08T17:01:56Z

+      });
+    })
+    .catch(err => {
+      reject(err);


Logger.log(err); throw err;

n-riesco · 2018-01-08T17:03:14Z

 import * as IbmDb2 from './ibmdb2';
 import * as ApacheLivy from './livy';
 import * as ApacheImpala from './impala';
+import * as DataWorld from './DataWorld';


Most of the project uses lowercase filenames and I'm planning to get rid of the exceptions.
Would you make this file lowercase?

n-riesco · 2018-01-08T17:06:53Z

+
+describe('Data World:', function () {
+
+    it('connect succeeds', function(done) {


You can return a promise and mocha will do the right thing:

+ it('connect succeeds', function() { + return connect(connection); + });

n-riesco · 2018-01-08T17:10:28Z

This looks very good. Please, see my feedback.

I don't have any more time today, but tomorrow I'm planning to search for the license of data.world's logo. If I don't find it, I'll contact them.

n-riesco · 2018-01-09T16:07:08Z

Screenshot showing use of data.world's logo:

n-riesco · 2018-01-09T16:41:41Z

I've found that the use of data.world's logo is governed by https://data.world/terms/ and they require written permission.

When I wrote the DB2 connector (IBM also requires written permission to use their logo), I decided to use the text DB2 instead of IBM DB2's logo. We could do the same with this connector, if we don't want to deal with requesting the written permission.

n-riesco · 2018-01-09T16:47:15Z

+    });
+
+    it('tables returns list of tables', function(done) {
+        tables(connection).then(result => {


return Promise

n-riesco · 2018-01-09T16:47:25Z

+    });
+
+    it('query returns rows and column names', function(done) {
+        query('SELECT * FROM sampletable LIMIT 5', connection).then(result => {


return Promise

n-riesco · 2018-01-09T16:47:59Z

+    });
+
+    it('schemas returns table schemas', function(done) {
+        schemas(connection).then(result => {


return Promise

kndungu · 2018-01-12T08:00:01Z

@tarzzz done.

n-riesco · 2018-01-12T13:07:21Z

💃

kndungu · 2018-01-12T15:59:00Z

💃 💃

kndungu force-pushed the add-data-world branch from c05c0d7 to 99cadf3 Compare January 2, 2018 15:16

kndungu force-pushed the add-data-world branch 6 times, most recently from 194bc00 to 2a497a7 Compare January 8, 2018 14:43

n-riesco reviewed Jan 8, 2018

View reviewed changes

n-riesco reviewed Jan 9, 2018

View reviewed changes

kndungu added 21 commits January 12, 2018 10:37

Replace - with _ on queries

11fae2e

Use query to get all tables

e0424ee

Remove redundant code

a18eee4

Use dataset URL

78ab1e0

Get tab label from provided URL

1c7b19d

Change sample dataset URL

5b58e41

Update text on connection tab

3148d22

Get database name from supplied URL

69a73b5

Add preview query

4f44902

Error handling

3d1b53c

Use single API call in schema function

9b510f9

Add a user agent to all API requests

17fb8e6

Remove duplicate code

563112f

Add data.world tests

a0d7021

Specify type of token

4805508

Remove unnecessary promises

87ff849

Replace dashes in tables and schemas function

2fbc1d5

Return promise in connect test case

1350ec8

Return promise in all data.world test cases

1ab33e8

Add data.world test script

04ad51a

Update filename

de82e14

kndungu force-pushed the add-data-world branch from c423a3a to de82e14 Compare January 12, 2018 07:39

n-riesco merged commit eecd62c into plotly:master Jan 12, 2018

kndungu deleted the add-data-world branch January 12, 2018 15:59

This was referenced Jan 19, 2018

Support data.world as a datasource #186

Closed

New Database Connectors #345

Open

n-riesco mentioned this pull request Apr 18, 2018

Support for Vertica #344

Open


		describe('Data World:', function () {

		it('connect succeeds', function(done) {

Uh oh!

Conversation

kndungu commented Jan 2, 2018

Uh oh!

n-riesco commented Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kndungu commented Jan 3, 2018

Uh oh!

kndungu commented Jan 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n-riesco Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n-riesco Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n-riesco commented Jan 8, 2018

Uh oh!

n-riesco commented Jan 9, 2018

Uh oh!

n-riesco commented Jan 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kndungu commented Jan 12, 2018

Uh oh!

n-riesco commented Jan 12, 2018

Uh oh!

kndungu commented Jan 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

n-riesco commented Jan 3, 2018 •

edited

Loading

n-riesco Jan 8, 2018 •

edited

Loading

n-riesco Jan 8, 2018 •

edited

Loading