Created ability to select specific columns #83

Closed
wants to merge 12 commits into
from

Conversation

Projects
None yet
2 participants
@jvskelton
Contributor

jvskelton commented Feb 28, 2014

Modified .all method to accept array of attributes

depending on the number of attributes given, the return type will be different. for example:

model.all({where: {name: 'bill'},attributes: ['id']},cb)
will return an array of ids(e.g. ['1','2', ..])

model.all({where: {address: '123'}, attributes: ['id', 'name']},cb)
will return [{id: 1, name: 'bill'}, {id: 2. name: 'alice'},...]

jvskelton added some commits Feb 20, 2014

added the ability to query specific columns, if the selected columns …
…length is one, it will return an array of values, vs an array of objects
Modified adapter to include .select to query specific columns, return…
…s one of three things, instance of model(if no attributes are provided), array of data(if single attribute is provided), or an array of objects(if multiple attributes are provided)
Update README.md
modified reference to travis ci image
modified .all to return array of object literals(or array of strings,…
… depending on the number of attributes) when attributes are given
test/basic-querying.test.js
@@ -42,6 +42,24 @@ describe('basic-query-mysql', function () {
});
});
+ it('should query collection with given attributes', function (done) {

This comment has been minimized.

Show comment Hide comment
@1602

1602 Mar 3, 2014

Contributor
  1. this method doesn't check actual logic of method you are testing. it's useless test. tests for this method should demonstrate at least: type of result, type of items in result (plain object, model or just value), acceptable value of attributes argument (i would suggest to allow fieldName (as string) to get array of values [1,2,3], and ['fieldName'] to get an array of plain objects [{id:1}, {id:2}, {id:3}]
  2. wrong indentation (should be 4 spaces)
@1602

1602 Mar 3, 2014

Contributor
  1. this method doesn't check actual logic of method you are testing. it's useless test. tests for this method should demonstrate at least: type of result, type of items in result (plain object, model or just value), acceptable value of attributes argument (i would suggest to allow fieldName (as string) to get array of values [1,2,3], and ['fieldName'] to get an array of plain objects [{id:1}, {id:2}, {id:3}]
  2. wrong indentation (should be 4 spaces)

This comment has been minimized.

Show comment Hide comment
@1602

1602 Mar 3, 2014

Contributor

And don't test select method in this repository. Only test code you've written, i.e. all method. When you add method to core - test it in core.

@1602

1602 Mar 3, 2014

Contributor

And don't test select method in this repository. Only test code you've written, i.e. all method. When you add method to core - test it in core.

- case 'JSON':
- val = JSON.parse(val);
- break;
+ case 'JSON':

This comment has been minimized.

Show comment Hide comment
@1602

1602 Mar 3, 2014

Contributor

Wrong indentation

@1602

1602 Mar 3, 2014

Contributor

Wrong indentation

lib/mysql.js
@@ -311,7 +311,16 @@ MySQL.prototype.escapeName = function (name) {
MySQL.prototype.all = function all(model, filter, callback) {
- var sql = 'SELECT * FROM ' + this.tableEscaped(model);
+ var sql = 'SELECT * '

This comment has been minimized.

Show comment Hide comment
@1602

1602 Mar 3, 2014

Contributor

This code is wrong. Should be like that:

var sql = 'SELECT '
if (filter.attributes) {
    sql += this.formatAttributesList(model, filter.attributes);
} else {
    sql += '*';
}
@1602

1602 Mar 3, 2014

Contributor

This code is wrong. Should be like that:

var sql = 'SELECT '
if (filter.attributes) {
    sql += this.formatAttributesList(model, filter.attributes);
} else {
    sql += '*';
}

This comment has been minimized.

Show comment Hide comment
@jvskelton

jvskelton Mar 3, 2014

Contributor

That is a much cleaner approach. Thank you for the suggestion

@jvskelton

jvskelton Mar 3, 2014

Contributor

That is a much cleaner approach. Thank you for the suggestion

README.md
@@ -1,4 +1,4 @@
-## JugglingDB-MySQL [![Build Status](https://travis-ci.org/jugglingdb/mysql-adapter.png)](https://travis-ci.org/jugglingdb/mysql-adapter)
+-## JugglingDB-MySQL [![Build Status](https://travis-ci.org/jugglingdb/mysql-adapter.png)](https://travis-ci.org/jugglingdb/mysql-adapter)

This comment has been minimized.

Show comment Hide comment
@1602

1602 Mar 3, 2014

Contributor

Why is it changed?

@1602

1602 Mar 3, 2014

Contributor

Why is it changed?

This comment has been minimized.

Show comment Hide comment
@jvskelton

jvskelton Mar 3, 2014

Contributor

nothing changed, i had thought i had the incorrect image displaying from travis CI

@jvskelton

jvskelton Mar 3, 2014

Contributor

nothing changed, i had thought i had the incorrect image displaying from travis CI

jvskelton added some commits Mar 3, 2014

created more tests to check the validity of the attributes param, mov…
…ed attribute string concat into dedicated function, fixed spacing issues
Update README.md
Included instructions on how to use attributes param to select specific columns and what the elementTypes of the returned array are.
@jvskelton

This comment has been minimized.

Show comment Hide comment
@jvskelton

jvskelton Mar 3, 2014

Contributor

Thank you very much for the feedback. I have made several modifications in line with what you have suggested.

Contributor

jvskelton commented Mar 3, 2014

Thank you very much for the feedback. I have made several modifications in line with what you have suggested.

@jvskelton jvskelton closed this Jul 1, 2014

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