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.

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

Initial implementation. #2

Merged
merged 2 commits into from
Jun 3, 2014
Merged

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented May 23, 2014

Add lib/connector.js and lib/sql.js from
loopbackio/loopback-datasource-juggler@5937f0c.

Export both classes from index.js

Rename BaseSQL to SqlConnector.

Fix jshint errors.

/to @raymondfeng @ritch please review

strongloop/loopback#275

Add `lib/connector.js` and `lib/sql.js` from
loopbackio/loopback-datasource-juggler@5937f0c.

Export both classes from `index.js`

Rename `BaseSQL` to `SqlConnector`.

Fix jshint errors.
@rmg
Copy link
Contributor

rmg commented May 23, 2014

Jenkins approves, but this PR existed before the job was added so it tested the branch directly instead of the PR: http://ci.strongloop.com/job/loopback-connector/1/

@slnode
Copy link

slnode commented May 23, 2014

All is well
Refer to this link for build results: http://ci.strongloop.com/job/loopback-connector/2/

@rmg
Copy link
Contributor

rmg commented May 23, 2014

My comment caught CI's attention.

@bajtos there should be a stub test at least:

var assert = require('assert');
var connector = require('../');
assert(connector.Connector);
assert(connector.SqlConnector);

@rmg
Copy link
Contributor

rmg commented May 23, 2014

FYI, none of the downstream jobs will be able to make use of this module in CI until this PR is merged into master, unless they have the same branch name.

@slnode
Copy link

slnode commented May 23, 2014

All is well
Refer to this link for build results: http://ci.strongloop.com/job/loopback-connector/4/

@ritch
Copy link

ritch commented May 28, 2014

Obviously the code is inherited from juggler, so thats not really the point of this review. My only concern is the lack of tests. Are there really none to port from juggler?

I think this is fine for now, but we should improve the tests.

@bajtos
Copy link
Member Author

bajtos commented May 29, 2014

Obviously the code is inherited from juggler, so thats not really the point of this review. My only concern is the lack of tests. Are there really none to port from juggler?

I did not find any. If you or Raymond can point me to some then I am happy to port them.

On the other hand, I am not sure what's the best way to test SqlConnector, since it's a base class to be inherited from. We can write a SQL connector in the test and run it agains a real SQL DB, but that seems like a duplication of other connectors. Or we can mock SqlConnector.prototype.query and write pure unit-tests only, asserting on the SQL commands being executed. That sounds like the best approach to me.

I think this is fine for now, but we should improve the tests.

Agreed. Can we leave writing new tests for another day and another pull request?

@raymondfeng
Copy link
Contributor

LGTM

@raymondfeng
Copy link
Contributor

At some point, I would like to add a connector for https://github.com/mapbox/node-sqlite3. It can provide a SQL based reference implementation using in-memory or file system store.

@bajtos
Copy link
Member Author

bajtos commented Jun 3, 2014

At some point, I would like to add a connector for https://github.com/mapbox/node-sqlite3. It can provide a SQL based reference implementation using in-memory or file system store.

Great idea. I suppose strongloop/loopback issues is the best place for tracking feature requests like this?

bajtos added a commit that referenced this pull request Jun 3, 2014
@bajtos bajtos merged commit d6d348f into master Jun 3, 2014
@bajtos bajtos deleted the feature/refactor-connector-base branch June 3, 2014 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants