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.

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

OpenRefine Database Import Extension #1394

Merged
merged 8 commits into from Jan 13, 2018

Conversation

5 participants
@tcbuzor
Copy link
Collaborator

tcbuzor commented Dec 23, 2017

No description provided.

TonyO added some commits Dec 23, 2017

*#
<html>
<head>
<title>Google Refine Database Extension v$version</title>

This comment has been minimized.

@thadguidry

thadguidry Dec 23, 2017

Member

Rename this with OpenRefine

<title>Google Refine Database Extension v$version</title>
</head>
<body>
<h1>Google Refine DATABASE Extension v$version</h1>

This comment has been minimized.

@thadguidry

thadguidry Dec 23, 2017

Member

Rename this with OpenRefine

@@ -0,0 +1,206 @@
/*
Copyright 2010, Google Inc.

This comment has been minimized.

@thadguidry

thadguidry Dec 23, 2017

Member

uhh, you probably want to put your name here and not Google's ? :)

@@ -0,0 +1,375 @@
/*
Copyright 2011, Google Inc.

This comment has been minimized.

@thadguidry
@@ -0,0 +1,52 @@
/*
Copyright 2011, Google Inc.

This comment has been minimized.

@thadguidry
@@ -0,0 +1,152 @@
/*
Copyright 2011, Google Inc.

This comment has been minimized.

@thadguidry
@@ -0,0 +1,34 @@
/*
Copyright 2011, Google Inc.

This comment has been minimized.

@thadguidry
@wetneb

This comment has been minimized.

Copy link
Member

wetneb commented Dec 24, 2017

@tcbuzor Many of the tests are empty - wouldn't it be better to provide actual test cases?

I will have some time to do a thorougher review next week - please do not merge yet!

@OpenRefine OpenRefine deleted a comment from codacy-bot Dec 24, 2017

@wetneb wetneb self-requested a review Dec 27, 2017

@wetneb
Copy link
Member

wetneb left a comment

From what I can see, it looks very nice, clean and usable. But, there are a few issues that should be addressed before this is merged.

Many unit tests are empty (already pointed out above). It is totally doable to test these features: Travis can run various databases easily, so we should take advantage of that.

I have tried to use the extension with a local postgres instance and I have run into a bug which could typically get caught by a unit test. I have input the SQL query "SELECT * FROM authors;" (with a semicolon at the end) and it generated exceptions in the backend that were not reported correctly to the user (I have got two javascript alerts with "undefined" as message). It looks like this is because your preview code adds a LIMIT at the end of the query in a way that breaks when there is a semicolon at the end. This should be made more robust.

Also, the values returned by a SQL queries are typed (string, int, timestamp, and so on): it would be cleaner if these were mapped to the corresponding data types in OpenRefine, rather than storing everything as text. (Maybe this could be enabled / disabled via a checkbox, in the same way that data type auto-detection works for CSV import).

There are also some minor UI issues - I will provide screenshots later on.

@wetneb

This comment has been minimized.

Copy link
Member

wetneb commented Jan 10, 2018

@tcbuzor any news on this?

@tcbuzor

This comment has been minimized.

Copy link
Collaborator Author

tcbuzor commented Jan 10, 2018

@codacy-bot

This comment has been minimized.

Copy link

codacy-bot commented Jan 13, 2018

Codacy Here is an overview of what got changed by this pull request:

Issues
======
- Added 54
           

See the complete overview on Codacy

@OpenRefine OpenRefine deleted a comment from codacy-bot Jan 13, 2018

@wetneb

This comment has been minimized.

Copy link
Member

wetneb commented Jan 13, 2018

@tcbuzor woaw, that's fantastic! so much testing! Thank you so much!

@wetneb

wetneb approved these changes Jan 13, 2018

@jackyq2015 jackyq2015 merged commit 4176dda into OpenRefine:master Jan 13, 2018

2 of 3 checks passed

Codacy/PR Quality Review Not so good... This pull request quality could be better.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@thadguidry

This comment has been minimized.

Copy link
Member

thadguidry commented Jan 13, 2018

@tcbuzor @jackyq2015 Any idea why all of those failed "loading workspace" under /tmp on Travis ??? I wonder if there's a setting in Travis we need to enable/disable ?

[testng] 04:58:35.883 [       FileProjectManager] Loading workspace: /tmp/OR_DBExtension_Test_WorkspaceDir5488033601648434206/workspace.old.json (0ms)
   [testng] 04:58:35.883 [       FileProjectManager] Failed to load workspace from any attempted alternatives. (0ms)
   [testng] .04:58:35.929 [       FileProjectManager] Overwriting singleton already set: com.google.refine.io.FileProjectManager@217abcc9 (46ms)
@wetneb

This comment has been minimized.

Copy link
Member

wetneb commented Jan 13, 2018

We need to disable these tests on Appveyor too, as Appveyor does not seem to support databases. Actually, we should disable the tests by default and only run them for Travis, so that folks don't have to launch three different database engines to test OpenRefine.

@thadguidry

This comment has been minimized.

Copy link
Member

thadguidry commented Jan 13, 2018

@wetneb

This comment has been minimized.

Copy link
Member

wetneb commented Jan 13, 2018

oh great! then why don't you configure appveyor so that it goes back to green? :-P

@thadguidry

This comment has been minimized.

Copy link
Member

thadguidry commented Jan 13, 2018

@wetneb because I'm retiring soon. :) And your going to run it all. :) :)

@wetneb

This comment has been minimized.

Copy link
Member

wetneb commented Jan 13, 2018

@thadguidry I'm so rusted on windozy-things that I was hoping I could ignore the issue ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment