<FromGitter>
<drujensen> I assigned the PR to the core and contributor teams. I just mentioned it here for others to be aware that lots of breaking changes are coming
_whitelogger has joined #amber
<FromGitter>
<OpakAlex> Hello guys, i am looking into granite, And i have one question (maybe so stupid), we have `src/granite/querying.cr` and `src/granite/query/builder.cr`. I think it maybe make sense if we will have all queries inside builder.cr, like first / last / select. Then we will have one abstraction and we can chain methods. What do you think?
<FromGitter>
<Blacksmoke16> they aren't the same thing
<FromGitter>
<Blacksmoke16> the former are the basic querying stuff, while the latter is for the query builder format
<FromGitter>
<Blacksmoke16> i.e. `Model.find 1` versus `Model.where(:id, :eq, 10).select`
<FromGitter>
<OpakAlex> but now, we can not have Model.all.where().order?
<FromGitter>
<Blacksmoke16> so? how is that different than `Model.where().order`?
<FromGitter>
<Blacksmoke16> obs if you dont include a conditional it'll return all the results
<FromGitter>
<OpakAlex> yes of course, i just thinking of eager_loading implementation to have it on top of src/granite/querying.cr and src/granite/query/builder.cr
<FromGitter>
<Blacksmoke16> i set your PR to draft, built in feature versus [DONT MERGE] in the title
<FromGitter>
<OpakAlex> added. check it please, because we need to have some container to add features like paginations, includes, etc.
<FromGitter>
<Blacksmoke16> sorry, i meant you dont need that in the title, because by virtue of the PR being a draft, it cannot be merged until you/admin marks it as ready
<FromGitter>
<OpakAlex> oh. sorry.
<FromGitter>
<Blacksmoke16> unfortunately i havent put a lot of thought into this feature before, so I dont really have any suggestions
<FromGitter>
<OpakAlex> But what about to have a wrapper for queries? This is my question
<FromGitter>
<OpakAlex> This is doest depends on feature
<FromGitter>
<Blacksmoke16> im not sure
<FromGitter>
<Blacksmoke16> assuming the API remains the same as it does now, could be worth doing. however i dont have any strong opinions either way
<FromGitter>
<OpakAlex> yes, this what i am trying to have same, API ;)
<FromGitter>
<Blacksmoke16> id be curious to see if that example works outside of spec context
<FromGitter>
<OpakAlex> have no idea what this means, i just see a lot of logs)
<robacarp>
@opakalex you mean crystal dumps some sort of stack overflow error or something?
<FromGitter>
<OpakAlex> yep
<robacarp>
can you add that to the issue?
<FromGitter>
<OpakAlex> i added last output of console
<robacarp>
ty
<robacarp>
I think this is probably fine, but at first glance I think the right way to fix it is to alter Granite::Columns::Type
<robacarp>
is this even supposed to handle an array?
<robacarp>
no wait
<robacarp>
you don't want to do this value.join business
<robacarp>
the reason it iterates the array and does an and for each value is to let sql do the join (and therefore defer responsibility of guarding against sql injection attacks)
<robacarp>
sorry just catching up
<robacarp>
yeah, you're mixing paradigms here
<FromGitter>
<OpakAlex> but interestion was wrong
<FromGitter>
<OpakAlex> yep
<FromGitter>
<OpakAlex> i am sure for this))
<robacarp>
the and(matches) method isn't for matching one of multiple values
<robacarp>
it's for matching all name=value pairs
<FromGitter>
<OpakAlex> make sense, but we have has of 3 keys
<robacarp>
if you want to match one of many values, you need an IN() builder
<robacarp>
and this should be def where(field : (Symbol | String), operator : Symbol, value : Array(Granite::Columns::Type))
<robacarp>
I don't remember if the IN() builder is there or not, it's been like 18mo since I wrote this
<FromGitter>
<OpakAlex> but can you create an PR? because for me it's so magic
<robacarp>
sorry, I can't... I'm swamped at work these days :(
<FromGitter>
<OpakAlex> maybe it's not infinity loop, but looks like this
<FromGitter>
<OpakAlex> if you run master with my spec you will see it
<FromGitter>
<robacarp> are you on Crystal 0.35?
<FromGitter>
<OpakAlex> yep
<FromGitter>
<OpakAlex> master green with 0/36
<FromGitter>
<robacarp> but the rest of the specs past?
<FromGitter>
<OpakAlex> this is new spec i added today
<FromGitter>
<OpakAlex> yes, all another looks good
<FromGitter>
<robacarp> but what I'm saying is there are a bunch of spec's that pass this behavior
<FromGitter>
<OpakAlex> not this.....it's only one with id: []
<robacarp>
your spec doesn't have that
<robacarp>
found = Review.where(id: [review1.id, review2.id]).select
<FromGitter>
<OpakAlex> yes: id: [Int32 | Int64]
<robacarp>
there are a couple dozen tests which check that an array can be passed to the where
<FromGitter>
<Blacksmoke16> key difference is this context is just `id: [review1.id, review2.id]`
<FromGitter>
<Blacksmoke16> and uses objects within the array, versus scalar values
<FromGitter>
<Blacksmoke16> granted i didnt look into this at all yet tho
<robacarp>
a object.id is just an Int, no magic there
<robacarp>
it might be an Int?
<robacarp>
is that the problem?
<robacarp>
it's an optional type?
<FromGitter>
<Blacksmoke16> everything is nilable
<FromGitter>
<Blacksmoke16> er but that getter is not nilable i think
<robacarp>
I think id isn't nilable
<FromGitter>
<Blacksmoke16> i would expect it to be `Int32` since they're both the same model
<FromGitter>
<Blacksmoke16> it has to be
<FromGitter>
<Blacksmoke16> doesnt have one until its saved
<robacarp>
I thought it raised if you tried to read it before save
<FromGitter>
<Blacksmoke16> not sure
<robacarp>
in any case, all the examples in the tests from last jan are literals, which aren't nilable
<FromGitter>
<OpakAlex> they are not nil
<robacarp>
okay, I don't think you mean `id: []` (query for id= empty array), I think you mean `id: Array(Int32?)` (query for id= one of the values in an array where they _may_ be nil)
<robacarp>
I don't see why that would cause an infinite loop...but I could easily see that being a bug
<robacarp>
when crystal sees an infinite loop, it says so explicitly: