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

Pass a list or array of columns to orderBy #8

Closed
elpete opened this issue Feb 10, 2017 · 3 comments
Closed

Pass a list or array of columns to orderBy #8

elpete opened this issue Feb 10, 2017 · 3 comments

Comments

@elpete
Copy link
Collaborator

elpete commented Feb 10, 2017

No description provided.

@timmaybrown
Copy link

timmaybrown commented Mar 21, 2017

Awesome project. I would like to try to tackle this issue if you wanted to provide some further direction on what you had in mind. My interpretation of the task is below.

Looking at the existing method signature it looks like the column argument is type of any. Would be pretty easy to pass a simple list or array to the method, iterate over the argument's value and append it to the variables.orders array.

Might be powerful if the column argument could also be allowed to pass an array-of-structs so each column that you are passing to have a different direction (asc vs desc) such as:

arOrderByCols = [ 
    { "col" = "last_name", "direction" = "asc" }, 
    { "col" = "age", "direction" = "desc" } 
];

or some sort of pipe delimited list:
colList = "last_name|asc,age|desc";

Let me know if I'm on the right track with what you were picturing.

I'm really trying to get my head around writing tests and this seemed like cool project to learn from and try to help out in the process.

@elpete
Copy link
Collaborator Author

elpete commented Mar 22, 2017

Thanks, Tim! You are 100% on the right track! One of the things I've tried to do with qb is to have many ways to accomplish the same task. That way you never feel like the tool is saying "You're doing it wrong!" With that in mind, I'd like to cover three additional use cases:

builder.from( "users" ).orderBy( [
    "last_name",
    [ "age", "desc" ],
    { column = "favorite_color", direction = "desc" }
] ).orderBy( "stars" );

would compile to

SELECT * FROM "users" ORDER BY "last_name" ASC, "age" DESC, "favorite_color", DESC, "stars" ASC

So I'd like to accept:

  • Just a column name. This would default to ascending.
  • An array, containing the column name and direction.
  • A struct with a column key and direction key.
  • BONUS: Your pipe example was neat as well. 😄

A couple other criteria I can think of:

  • If an array is passed as the first argument, just ignore the second argument direction.
  • The order in the array passed in is the order added to the query.
  • An subsequent orderBy calls should just append in order (like the above example).
  • Any additional functionality should be documented in the doc blocks. (This is all the documentation that currently exists. 😞 )

Hopefully that doesn't scare you away! I think this is a great feature to work on TDD with, like you said. You can find the existing orderBy tests here. Most of the tests follow this general pattern:

  1. Get a new builder getBuilder().
  2. Call the methods you are testing.
  3. Call toSQL() on the builder to get the generated SQL.
  4. Compare that SQL to what it should be.
  5. If bindings were added, compare they were added in the correct order, as well.

Feel free to push early and often and ask questions along the way. You don't need to finish with all of it before opening a pull request. Heck, GitHub says they open pull requests before they start coding sometimes!

Thanks again, and I'm excited to work with you!

timmaybrown pushed a commit to timmaybrown/qb that referenced this issue Mar 23, 2017
…-modules#8 to accept an array or list as the column argument's value. The array can accept a variety of value formats that can be intermingled if desired. All scenarios will inherit eithe the default direction or the supplied value for the direction argument.
timmaybrown pushed a commit to timmaybrown/qb that referenced this issue Mar 23, 2017
…array, removed lists as a valid value for array value and refactored validDirections array to be an instance variable aptly named to match the other naming conventions.
elpete pushed a commit that referenced this issue Mar 23, 2017
…ccept an array or list as the column argument's value. The array can accept a variety of value formats that can be intermingled if desired. All scenarios will inherit eithe the default direction or the supplied value for the direction argument.
elpete pushed a commit that referenced this issue Mar 23, 2017
…lists as a valid value for array value and refactored validDirections array to be an instance variable aptly named to match the other naming conventions.
@elpete
Copy link
Collaborator Author

elpete commented Mar 23, 2017

Feature available in v2.1.0. Thanks again, @timmaybrown!

@elpete elpete closed this as completed Mar 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants