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

Require ResultSet to just implement read, optionally implementing read(T.class). Fixes #5 #9

Merged
merged 1 commit into from
Jun 30, 2016

Conversation

asterite
Copy link
Member

The scalar method now also returns a union, and one can cast it to the needed type.

asterite pushed a commit to crystal-lang/crystal-sqlite3 that referenced this pull request Jun 28, 2016
asterite pushed a commit to crystal-lang/crystal-mysql that referenced this pull request Jun 28, 2016
@asterite asterite mentioned this pull request Jun 28, 2016
read?(t).not_nil!
# Reads the next column value as a **type**
def read(type : Class)
type.cast(read)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • shouldn't this base implementation simplify the code of dummy_driver#read(T) ?
  • does Class match structs and unions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a lot. I think the dummy driver can just have all the values as an array of DB::Any. I'll try :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Class matches any type, even union types.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bcardiff I don't know how to easily simplify the specs, I can see DummyResultSet receives the query and splits it by commas. I'd leave it like this for now so read(Class) is also tested.

@asterite asterite force-pushed the feature/simple_read branch 3 times, most recently from 2b165bb to 64140f1 Compare June 29, 2016 15:40
@asterite
Copy link
Member Author

asterite commented Jun 29, 2016

I've just updated this PR with some additions. Sample code:

DB.open "sqlite3://%3Amemory%3A" do |db|
  db.exec "create table contacts (name string, age integer)"
  db.exec "insert into contacts values (?, ?)", "John Doe", 30

  # `read(*types)` to read multiple values at once
  db.query "select name, age from contacts" do |rs|
    rs.each do
      name, age = rs.read(String, Int32)
      pp name, age
      # name # => "John Doe"
      # age  # => 30
    end
  end

  # `query_one` to read a single row
  # This raises if there are no rows, or more than one row
  name, age = db.query_one "select name, age from contacts", &.read(String, Int32)
  pp name, age
  # name # => "John Doe"
  # age  # => 30

  # Another way:
  name, age = db.query_one "select name, age from contacts", as: {String, Int32}
  pp name, age
  # name # => "John Doe"
  # age  # => 30

  # Another way, just read the name:
  name = db.query_one "select name from contacts", as: String
  pp name
  # name # => "John Doe"

  # similar to `query_one`, but returns nil if there are no rows (still raises if there are many rows)
  result = db.query_one? "select name, age from contacts", &.read(String, Int32)
  pp result, typeof(result)
  # result         # => {"John Doe", 30}
  # typeof(result) # => (Tuple(String, Int32) | Nil)

  # Another way:
  result = db.query_one? "select name, age from contacts", as: {String, Int32}
  pp result, typeof(result)
  # result         # => {"John Doe", 30}
  # typeof(result) # => (Tuple(String, Int32) | Nil)

  # Another way, just read the name:
  name = db.query_one? "select name from contacts", as: String
  pp name, typeof(name)
  # name         # => "John Doe"
  # typeof(name) # => String | Nil

  db.exec "insert into contacts values (?, ?)", "Foo Bar", 40

  # `query_all` to read an array of values
  people = db.query_all "select name, age from contacts order by age desc", &.read(String, Int32)
  pp people, typeof(people)
  # people         # => [{"Foo Bar", 40}, {"John Doe", 30}]
  # typeof(people) # => Array(Tuple(String, Int32))

  # Another way:
  people = db.query_all "select name, age from contacts order by age desc", as: {String, Int32}
  pp people, typeof(people)
  # people         # => [{"Foo Bar", 40}, {"John Doe", 30}]
  # typeof(people) # => Array(Tuple(String, Int32))

  # Another way, just read the name:
  names = db.query_all "select name from contacts order by age desc", as: String
  pp names, typeof(names)
  # names         # => ["Foo Bar", "John Doe"
  # typeof(names) # => Array(String)
end

This fixes #10 in a much better way.

And the nice thing about query_one, query_one? and query_all is that the returned value depends on the block, so one can create an instance of a class as a result and, for example, get an array of proper classes instead of an array of tuples. Or use named tuples, etc.

I think with this we could easily migrate crystal-pg to this and have the same functionalities, even with a nicer syntax.

EDIT: I think the scalar method is a bit redundant now, you can do query_one "...", &.read for the same behavior.

/cc @bcardiff @waj @spalladino

@asterite asterite force-pushed the feature/simple_read branch 4 times, most recently from 3927fab to c3d1d64 Compare June 29, 2016 22:25
@bcardiff
Copy link
Member

  1. #scalar should be available in query methods. is shorter and useful. but it can be implemented in QueryMethods and not in Statement for sure.
  2. Can't we have query_one sql, T1, T2 ... TN and be type safe? It would be a better syntax IMO. Or is there something I am missing? I can't imagine other than a read what I will want to have. Maybe the things we would miss there is the named tuple support.

But other than that I think we can merge this. :-) I like it.

@asterite
Copy link
Member Author

@bcardiff The problem is that query_one also accepts the arguments, like:

db.query_one "select name from contacts where id = ? limit 1", 123, &.read(String)
#                                                              ^-- argument 

so it would be hard to distinguish between arguments and types.

@bcardiff
Copy link
Member

if only something like this would work...

db.query_one {String, String}, "select name, email from contacts where id = ? limit 1", 123
db.query_one "select name, email from contacts where id = ? limit 1", 123, as: {String, String}

But well, let's keep the &.read(String) until a better alternative comes.

@asterite
Copy link
Member Author

asterite commented Jun 30, 2016

@bcardiff The first form, query {String, String}, "select ...", args is how you do things in crystal-pg right now. But I'm not sure it reads very well.

I like the as form, however, it reads much better: first the query, then the bind arguments, then the expected type (which is marked as as, so reminds us of a regular cast).

I've updated the PR with those forms, and updated the sample code in the comment above.

@asterite
Copy link
Member Author

My only "problem" or question with the as argument is this:

# This is OK, returns a string and an int32
db.query_one "...", as: {String, Int32}

# What about this?
db.query_one "...", as: Tuple(String, Int32)

Right now the second way doesn't compile, because that ends up calling rs.read(Tuple(String, Int32)) and you can't read a tuple from a column. Maybe that's fine. But one should remember to always use a tuple as an as argument, not a tuple type (probably not a problem because it's easier to write {...} than Tuple(...))

Of course if a DB driver allows you to read a tuple from a single column, the second form will work.

@asterite
Copy link
Member Author

I also left the block versions in all cases, because you can then do:

point = db.query_one("select lat, lng from locations where id = ?", 123) { |rs| Point.new(*rs.read(Float64, Float64)) }
point # => Point(...)

@bcardiff
Copy link
Member

It's nice how {String, Int32} is of type Tuple(String:Class, Int32:Class) and then we can instantiate the method for that specific type. :-)

@asterite asterite merged commit 344804d into master Jun 30, 2016
@asterite asterite deleted the feature/simple_read branch June 30, 2016 19:13
asterite pushed a commit to crystal-lang/crystal-sqlite3 that referenced this pull request Jul 1, 2016
asterite pushed a commit to crystal-lang/crystal-mysql that referenced this pull request Jul 1, 2016
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.

2 participants