<kares>
rdubya: around? I'll post some of the comment feedback here
<kares>
> Some of the tests this fixes are ones that are failing because ActiveRecord doesn't have the type information when the AR::Result is returned. It also had the side effect of fixing some of the bc timestamp issues because now it returns a string for dates, not sure if that is a good tradeoff or not.
<kares>
that is than maybe a regression - question is which is less of a trade-off
<kares>
my big PR addressed (hardest part) returning typed info - simply because this has been a source of confusion for users in 1.3
<kares>
e.g. `SELECT now()` is expected to return a Time not a String
<kares>
BC should be close to working - there's some bugs in JRuby itself that prevent it from working (better on master) ... did some work lately on 50-stable and it should be okay-ish
<kares>
> I'm also a little confused about your comment about having the 3 different types, don't we essentially already have that? The arjdbc "types" are just defined as methods right now instead of objects, right?
<kares>
well it seemed like too much type systems to me :) ... fully understand that it likely makes sense for PG
<kares>
not sure for others - e.g. MySQL. feel like there would be more implementation specific details than now
<kares>
regarding execute - that is an internal method, which never returned the same piece between adapters
<kares>
so I would not worry there at all - it actually used to return raw Hash-like data (not AR::Result)
<kares>
so that is something you guys did with the refactoring and I am not sure how much it matters
<kares>
SUMMARY: I am in no position to clearly decide at this point whether its a bad direction but if possible I would not force it for all since it feels PG-motivated (and some of the introduced types are actually PG driver specific e.g. the ObjectType)
<enebo>
kares: there are generic types defined in his PR but I do not see them hooked up to anything but pg-specific adapter code
<enebo>
kares: I know almost nothing about pg adapter in arjdbc or ar at this point so I cannot comment on this too much
<enebo>
kares: I would say for mysql and sqlite3 we probably don't need an abstract boxed type
<enebo>
kares: no doubt pg native adapter does have a similar wrapped data type since they have a crazy amount of extended features
<kares>
enebo: yes I agree that would be a good start for rdubya maybe
<enebo>
kares: but do you see this used by anything but pg?
<kares>
well he did it in a way that's generic so it could be but I would take smaller steps
<enebo>
kares: perhaps if it is just pg we can move from jdbc/types to be within postgresql namespace
<kares>
what I am not sure about is how much that hustle matters
<enebo>
kares: I would not do it at all for those other two adapters
<enebo>
kares: at least the native adapters in AR do not do extra boxing
<kares>
for starters I agree
<kares>
also as pointed out - I encourage you folks to not make a mantra out of rails' tests :)
<kares>
e.g. that execute piece - does not matter what it does, that used to be a very adapter specific return type
<enebo>
kares: well personally I would like to delete 80% of our local tests and only test what is JDBC-specific behavior
<enebo>
kares: every time Rails changes something significant we update our code and our local test suite for tests which AR already has
<kares>
I still find AR's tests valuable as a primary indicator
<kares>
I mean AR-JDBC's
<kares>
yes but those changes are in major versions only
<enebo>
kares: yeah but why do them at all
<kares>
and we need to test some extended features or edge cases ...
<enebo>
kares: agreed
<enebo>
kares: for any extended features or untested edge cases we definitely need local tests
<enebo>
for anything covered by base_test.rb (I think that is the name) I don't think those tests should be copied into our test suite
<kares>
some/maybe most of it can go, but I am in no position to tell which ones as I am not familiar with the Rails suite
<enebo>
kares: yeah so I think we can try to take this on a little at a time
<kares>
... and rather spent time elsewhere :)
<enebo>
kares: I have seen a number of duplicated tests. some a tiny bit different but that is just drift of AR tests from historical tweaks by AR people
<enebo>
kares: when we see a simple obvious behavior like testing a basic insert we should just defer to AR tests
<enebo>
kares: but only when it is totally obvious AR has reasonable and simple coverage of it
<kares>
okay no problem at my end but with one assumption
<kares>
we get those rails targets green-ish (with excludes)
<kares>
... planning to look into that - hoopefully next week
<enebo>
kares: yeah so my big problem is getting minitest-excludes to even work on travis
<enebo>
kares: for the two adapters I was working on I had very very few things to exclude from either 5.0.x or 5.1.x
<kares>
it is not working?
<enebo>
kares: like <10
<enebo>
kares: sorry I apologize for not running it. Everything takes a lot longer than I think
<kares>
yeah its mostly PG - that is tested extensively
<enebo>
kares: I will try once I finish my ripper/lexer changes for 2.5
<kares>
... and maybe rdubya is spending too much effort getting lost in raising compatibility :)
<enebo>
kares: well I am not sure I understand that comment
<kares>
which is a noble thing but not sure if its worth to hustle too much - on the other hand I do not want to discourage him
<enebo>
kares: which compatibility? with AR?
<kares>
yep
<kares>
from some of the failures
<kares>
although he did a great job already with excludes now that I look into master
<kares>
seems like master is not in sync with 50-stable
<enebo>
kares: well I would like to support all goals here :) but we should isolate specific things we will not be compatible with and then make an issue and then document them
<enebo>
kares: if it really does not make sense to follow AR native behavior I think we need much better delineation of that
shellac has joined #jruby
<enebo>
kares: like the excludes themselves can refer to an issue and the issue can refer to something which explains why we are not doing it the same
<kares>
maybe it does - I might be motivated too much to spent as little time as possible :)
<enebo>
kares: I am thinking about this in the sense that if we want some new hackers getting on board, they don't know why we made some of these decisions
<enebo>
kares: yeah I totally get that too :)
<kares>
well ROb did that with the excludes
<enebo>
kares: My other motivation is that if I look at a problem 9 months from now I know something I can reference
<kares>
so there's a seed of having it done nicely
<enebo>
kares: yeah I think excludes is a really good hook for us to be the start of the bread crumbs to why we are not passing
<enebo>
but of course excludes need to work. I wonder how hard it would be to get rails to just include it...but I think zenspider has never updated that gem
<enebo>
hahaha he appears to have just did that and ignored the PR
<enebo>
oh but still has not released
<enebo>
gah
<enebo>
oh he did assign himself on jan so he did ackownledge that PR
<kares>
will merge the auto-building of ext .jar - its not without ever commit-ing the .jar as you guys wanted
<enebo>
kares: well it will be less commits for now anyways
<enebo>
kares: but yeah never committing would be nice. Since either I or you reelase we both have jdk installed
<kares>
and not being lost in translation I mean branches
<enebo>
kares: yeah
<kares>
if you guys hit any issues just let me know - most of it should be tested fairly well
<enebo>
kares: I sent Ryan a tweet asking about a release
<kares>
kk
<enebo>
kares: getting that for reals released may allow me to get Rails to add it to their Gemfile
<enebo>
kares: that is not a short term soln but if we get it in then we will be able to have a much simpler rake task
<enebo>
I tried so many things to get it to properly load on travis...very frustrating :)
<kares>
really not sure but I thought it worked
<rdubya>
enebo: kares: sorry, I missed the beginning of this conversation so some of this may come out disjointed
<rdubya>
I think one of my goals has been to try to get the arjdbc adapters and the AR native adapters to be as close as possible on compatibility because I was under the assumption that this gem should be a drop in replacement
<rdubya>
if somebody wants to switch from MRI to JRuby, its a lot easier if they don't have to have conditionals based on which library they are running
<rdubya>
maybe that is a bad assumption though
<enebo>
rdubya: I think it is the right general goal but there may be two reasons to deviate on occasion: 1) extensions not supported by those native adapters 2) improving performance
<kares>
in general yes but if its too much effort e.g. #execute wasn't worth to fix as there's no consistency in that
<enebo>
rdubya: I think both cases need to be well-documented and justified though
<kares>
others exec_xxx are worth it
<kares>
guys I managed to broke CI in 50-stable and have to run shortly, will get to it tomorrow or so
<enebo>
but on the general goal if it eliminates a test we have to maintain it is also worth it
<enebo>
kares: yeah np
<rdubya>
that was my goal with these changes though, to provide some consistency to the arjdbc Result object that comes back from execute so that we can have adapter specific results built on those that can align with the ones from the MRI side
<rdubya>
and I *think* is only adding one object creation in most cases, but I need to go back and verify that
<kares>
rdubya: if you could for now make that code PG specific - e.g. move the type package
<kares>
than I think we could merge but for now I would not try pushing smt similar for others
<kares>
which means some code duplication? as you refactored the methods into classes ...
<kares>
you should setup one ext point where the 'new' type system would be hooked up - should be possible
xardion has quit [Remote host closed the connection]
<rdubya>
I'll see what I can do, the bulk of that pr is just moving logic from the booleanToRuby method to the BooleanType#extractRubyObject method
<rdubya>
so there would still be a decent chunk of duplication
xardion has joined #jruby
<enebo>
I will review this but I got a better informal description of how this works and I like the idea behind it
<enebo>
I can see using it from both mysql and sqlite3 now as well
bbrowning has quit [Ping timeout: 260 seconds]
bbrowning has joined #jruby
shellac has quit [Quit: Computer has gone to sleep.]
shellac has joined #jruby
shellac has quit [Quit: Computer has gone to sleep.]
xardion has quit [Ping timeout: 264 seconds]
xardion has joined #jruby
jrafanie has quit [Quit: My MacBook has gone to sleep. ZZZzzz…]
Puffball has quit [Remote host closed the connection]
jrafanie has joined #jruby
Puffball has joined #jruby
Puffball has quit [Remote host closed the connection]
jrafanie has quit [Quit: My MacBook has gone to sleep. ZZZzzz…]
<GitHub71>
[jruby] nomadium opened pull request #5106: Fix RubyString instances to raise FrozenError exceptions when mutations are attempted in debug mode… (ruby-2.5...fix-string-tests-with-frozen-error-related-failures) https://git.io/vxBv6
rtyler has joined #jruby
<desnudopenguino>
is anyone using jruby with rbenv? i'm having trouble installing it, getting java 7 is required (i have java 8 installed)
claudiuinberlin has joined #jruby
<desnudopenguino>
bah, i just removed the java 7 check.
sidx64_ has quit [Ping timeout: 260 seconds]
<GitHub172>
[jruby] enebo closed pull request #5106: Fix RubyString instances to raise FrozenError exceptions when mutations are attempted in debug mode… (ruby-2.5...fix-string-tests-with-frozen-error-related-failures) https://git.io/vxBv6
<GitHub121>
[jruby] enebo pushed 2 new commits to ruby-2.5: https://git.io/vxBkC
<GitHub121>
jruby/ruby-2.5 5799d96 Thomas E Enebo: Merge pull request #5106 from nomadium/fix-string-tests-with-frozen-error-related-failures...
<GitHub121>
jruby/ruby-2.5 fdbc404 Miguel Landaeta: Fix RubyString instances to raise FrozenError exceptions when mutations are attempted in debug mode...