refactor the querying functions into their own module

[mmmries]

I was reading through your PR tonight and I was wondering if it was possible to reorganize the query logic into a pipeline rather than a set of nested function calls.

I'm not really sure if this is actually an improvement on the code, but I'm curious to get your feedback.

Things I Like

• most of the pattern matching has been pushed into function clauses rather than case statements
• query logic has been separated into its own module
• Sqlitex.query/2 is now a very readable pipeline, easier to dive into than walking through the nested function calls that previously existed

Things I Don't Like

• Sqlitex.Query.bind_values/2 has to use a case statement to pass along the statement so we can keep it passing through the pipeline
• Sqlitex.Query.to_rows/1 takes a single tuple with four items, instead of four arguments. Not a huge deal, but I would prefer 4 explicit arguments.
• When an error occurs, the errors gets passed through the rest of the pipeline before coming back to the user, rather than returning early and avoiding extra function calls

/cc @jazzyb

[jazzyb]

My initial opinion after having just glanced over the changes is that I like that you are splitting up some of the logic. It does make Sqlitex.query/2 look cleaner, and I do like moving matching logic to functions as opposed to case statements.

The problem I see -- and I have run into this in my own Elixir code -- is that the new Sqlitex.Query module doesn't really stand on its own. What I mean by that is that with the exception of prepare/2, I can't open up that module and get a feel for what it is doing -- I have to see the context of how it is being used by the Sqlitex module. If none of those exported functions are general enough to be used alone or for other purposes, does it make sense to have a separate module for them?

What I suggest is either:

1. Leave the Sqlitex.Query functions as you have written them, but move them back to the top-level Sqlitex module as private functions, OR
2. Move the query/3 function to Sqlitex.Query as the only exported function and have a wrapper function Sqlitex.query/3 simply call Sqlitex.Query.query/3.

On the topic of passing errors through the pipeline: I really wish Elixir had a "fail early" option for pipes. I don't know how that would be implemented... probably with macros somehow.

I'll take a closer look tomorrow after some sleep.

[jazzyb]

You said you dislike the tuple here. I don't think it makes too much of a difference, but could you prepend everything with the following match:

def to_rows({types, cols, rows, into}), do: to_rows(types, cols, rows, into)

and then deal with four separate arguments in the remaining functions?

EDIT: Just to clarify: I didn't mean that as a request, just a suggestion if you prefer 4 arguments over tuples.

[mmmries]

That is a really great insight about Sqlitex.Query. I couldn't put my finger on why I felt a little uncomfortable with the structure there, but having public functions that aren't really intended to be used publicly is definitely a part of what I was feeling.

Between your two suggested solutions I think I prefer to have a public Sqlitex.Query.query/2 public function and make the rest of the module public. Then Sqlitex.query/2 can just delegate to Sqlitex.Query.query/2. That keeps all of the context about how a query happens in one place without mixing it up with logic about how to create tables or execute raw sql statements.

[mmmries]

Talking to @film42 this morning, he mentioned that this looks a lot like carrying a MaybeMonad through our pipeline. That gave me the idea of using a macro that checks for {:error, _} tuples through the pipeline and I found Elixir Pipes by Bruce Tate which already does this.

I think I'm going to try rewriting the pipeline one more time with that macro to see if I like the code better

[jazzyb]

Ooooooo, good find. I think I love elixir-pipes.

[mmmries]

Here is an implementation that is using the pipe library and it has moved all of the query logic into the Sqlitex.Query module. I'm reasonably happy with this implementation. What do you think?

[mmmries]

I got around the issue of having a different interface between execute and to_rows by making execute call directly into to_rows. That way the whole function either returns {:ok, results} or {:error, some_error}. Not a brilliant solution, but it works well enough.

[mmmries]

I took one more swing at this before I go to bed and I'm not sure if I like it better or not. Basically instead of passing along large tuples, I created Sqlitex.Query struct that has places for all the various intermediate values and then I made points in the pipeline for each of the steps that was being handled by execute previously.

This makes the code a bit more verbose (more total lines). But I do like the fact that all the error checking is happening right by where the errors occur. This means let me get rid of a lot of function clauses that were checking for {:error,_} patterns and the to_rows function no longer handles the special cases of {:error, :no_columns}.

[mmmries]

Travis is failing on the build now, maybe it has to do with the fact that Travis is using elixir 1.0.2

I'll have to check tomorrow, too sleepy to make progress now 😴 😪 💤

[jazzyb]

I love the changes so far. I love that the return_rows_or_error mess that I foisted upon you before has been enormously simplified. I'm even fine with the :ok matches since they are within private functions now. It may be more total lines, but (once you know what pipe_matching does) it is easier to see what each individual function's responsibility is.

Sadly, the elixir-pipes guy hasn't had any activity on Github for a month :( I don't know when we might hear back from him on the hex package thing. You could reference the github page directly:

{:pipe, git: "https://github.com/batate/elixir-pipes"}

But at that point it might be best not to include the library at all. Maybe you could just copy/paste the functionality you need from elixir-pipes until hex is updated?

As far as some of the Travis CI build failures are concerned, kiex, which they use to manage Elixir versions, pulls the Elixir releases straight from Github. When Github gets too many requests from the same IPs, it begins throttling access. Keep an eye out on that when you get Travis errors. See: travis-ci/travis-ci#3269

[jazzyb]

I just learned about defdelegate yesterday:

defdelegate query(db, sql, opts \\ []), to: Sqlitex.Query

Doesn't make much of a difference here, but it is great when you are wrapping a LOT of functions with many arguments.

[mmmries]

👍 I never knew about that. I'll give it a try

[mmmries]

pipes 0.0.2 got released today and it specifies support for Elixir 1.0.

So that problem is solved. I'll try to see if I can make the pipeline match on {:ok, _}.

The github links for precompiled Elixir releases are all backed by AWS links. It seems like maybe kiex should skip github altogether, but I won't try to tackle that problem. I'll just try to re-run the build during a calm time of day.

Thanks again for all your help. I'm learning a ton of good Elixir from this project.

[mmmries]

Woohoo! the more generic {:ok,_} pattern worked fine kiex managed to install all the version of elixir: https://travis-ci.org/mmmries/sqlitex/builds/64582808

[jazzyb]

Looks great! Let me know if there are any final changes you would like to make. Otherwise, I'll pull it in tonight and finalize my PR.

[mmmries]

I think I'm happy with this. Feel free to merge my pull into your pull and your pull into master!

This pull request inception will be done!