Select specific cols(issue #143) #384

Closed
wants to merge 16 commits into
from

Conversation

Projects
None yet
3 participants
@jvskelton
Contributor

jvskelton commented Feb 28, 2014

  • created model.select to query specific columns
  • either returns array of data or array of objects(see README)
  • error handling for adapters that have not implemented the functionality(currently only supported in
    jvskelton/jugglingdb-myql until merge).
@jvskelton

This comment has been minimized.

Show comment Hide comment
@jvskelton

jvskelton Feb 28, 2014

Contributor

*correction jvskelton/mysql-adapter

Contributor

jvskelton commented Feb 28, 2014

*correction jvskelton/mysql-adapter

@1602

This comment has been minimized.

Show comment Hide comment
@1602

1602 Feb 28, 2014

Owner

I would suggest to implement it in a different way: in mysql adapter, adapter.all method you can check for filter.attributes and replace * in query with the list of attributes when filter.attributes is present, no reason for adding another core method which will work only for limited set of adapters, especially in this particular case when it can be implemented by extending existing feature.

Owner

1602 commented Feb 28, 2014

I would suggest to implement it in a different way: in mysql adapter, adapter.all method you can check for filter.attributes and replace * in query with the list of attributes when filter.attributes is present, no reason for adding another core method which will work only for limited set of adapters, especially in this particular case when it can be implemented by extending existing feature.

@1602 1602 closed this Feb 28, 2014

@jvskelton

This comment has been minimized.

Show comment Hide comment
@jvskelton

jvskelton Feb 28, 2014

Contributor

My only concern with that however, would be the fact that currently, I return either an array of data(if one attributed is given) or an array of object literals(if more the one attribute is given), which means the return type would be different. I felt that returning a model instance with potentially several undefined values, along with the model specific functions attached was a bit redundant(not to mention could cause problems if that instance was saved). What are your thoughts on that?

Contributor

jvskelton commented Feb 28, 2014

My only concern with that however, would be the fact that currently, I return either an array of data(if one attributed is given) or an array of object literals(if more the one attribute is given), which means the return type would be different. I felt that returning a model instance with potentially several undefined values, along with the model specific functions attached was a bit redundant(not to mention could cause problems if that instance was saved). What are your thoughts on that?

@anatoliychakkaev

This comment has been minimized.

Show comment Hide comment
@anatoliychakkaev

anatoliychakkaev Feb 28, 2014

Collaborator

I think that feature makes sense. But this is nothing just pure jugglingdb
feature, it should call adapter.all method and return plain objects /
strings. No changes in adapters required at all. So, to conclude, there are
two features:

  1. in mysql adater: hook up 'attributes' property
  2. in jugglingdb core: new method which always returns plain objects or
    just value depending on what you want. this method should work with
    'attributes' feature of adapter (when adapter supports it), or just do it
    manually, as _.pick works in undercore.js

On 28 February 2014 16:29, jvskelton notifications@github.com wrote:

My only concern with that however, would be the fact that currently, I
return either an array of data(if one attributed is given) or an array of
object literals(if more the one attribute is given), which means the return
type would be different. I felt that returning a model instance with
potentially several undefined values, along with the model specific
functions attached was a bit redundant(not to mention could cause problems
if that instance was saved). What are your thoughts on that?

Reply to this email directly or view it on GitHubhttps://github.com/1602/jugglingdb/pull/384#issuecomment-36368033
.

Collaborator

anatoliychakkaev commented Feb 28, 2014

I think that feature makes sense. But this is nothing just pure jugglingdb
feature, it should call adapter.all method and return plain objects /
strings. No changes in adapters required at all. So, to conclude, there are
two features:

  1. in mysql adater: hook up 'attributes' property
  2. in jugglingdb core: new method which always returns plain objects or
    just value depending on what you want. this method should work with
    'attributes' feature of adapter (when adapter supports it), or just do it
    manually, as _.pick works in undercore.js

On 28 February 2014 16:29, jvskelton notifications@github.com wrote:

My only concern with that however, would be the fact that currently, I
return either an array of data(if one attributed is given) or an array of
object literals(if more the one attribute is given), which means the return
type would be different. I felt that returning a model instance with
potentially several undefined values, along with the model specific
functions attached was a bit redundant(not to mention could cause problems
if that instance was saved). What are your thoughts on that?

Reply to this email directly or view it on GitHubhttps://github.com/1602/jugglingdb/pull/384#issuecomment-36368033
.

@jvskelton

This comment has been minimized.

Show comment Hide comment
@jvskelton

jvskelton Jun 27, 2014

Contributor

@anatoliychakkaev This is a bit delayed, but just to clarify, you do or do not want a new method in the jugglinddb core(which is currently how it works). I believe the last comment conflicts with your first comment. Thanks.

Contributor

jvskelton commented Jun 27, 2014

@anatoliychakkaev This is a bit delayed, but just to clarify, you do or do not want a new method in the jugglinddb core(which is currently how it works). I believe the last comment conflicts with your first comment. Thanks.

@1602

This comment has been minimized.

Show comment Hide comment
@1602

1602 Jun 28, 2014

Owner
  1. new method in jugglingdb which will call .all with {attributes:[]} property and also ensure that returned dataset only includes required attributes (removing all unexpected attributes).
  2. in adapter: hook up attributes property in adapter.all method

my comments are not in conflict. first comments meaning is that we don't need additional method - all you need is just add support of 'attributes' clause in adapter.all method. second comment suggests to add core method for convenience and cross-adapters compatibility, so that this feature will work in the same way in all adapters not only adapters with 'attributes' clause support in adapter.all method. but you have to filter object manually in case if it contains unexpected attributes. hope that makes sense.

Owner

1602 commented Jun 28, 2014

  1. new method in jugglingdb which will call .all with {attributes:[]} property and also ensure that returned dataset only includes required attributes (removing all unexpected attributes).
  2. in adapter: hook up attributes property in adapter.all method

my comments are not in conflict. first comments meaning is that we don't need additional method - all you need is just add support of 'attributes' clause in adapter.all method. second comment suggests to add core method for convenience and cross-adapters compatibility, so that this feature will work in the same way in all adapters not only adapters with 'attributes' clause support in adapter.all method. but you have to filter object manually in case if it contains unexpected attributes. hope that makes sense.

@jvskelton

This comment has been minimized.

Show comment Hide comment
@jvskelton

jvskelton Jun 30, 2014

Contributor

Ah, ok that makes sense, thank you for clarifying.

Contributor

jvskelton commented Jun 30, 2014

Ah, ok that makes sense, thank you for clarifying.

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