Add Ruby 3.0 and Ruby 3.1 to the CI matrix#315
Add Ruby 3.0 and Ruby 3.1 to the CI matrix#315flavorjones merged 5 commits intosparklemotion:masterfrom petergoldstein:feature/add_ruby_3_0_and_3_1
Conversation
|
👍 , I just ran into the same thing. cc @tenderlove @flavorjones, would be nice to merge this. |
|
I've kicked CI to demonstrate green but I've not been involved with sqlite3 in 12 years. I'd like someone who's more familiar with sqlite3 and this code to determine whether the downcase fixes for column types is the right thing to do (since it's not clear to me why or when this behavior changed). @tenderlove ? |
|
|
||
| def test_types | ||
| assert_equal [ "integer", "text" ], @result.types | ||
| assert_equal [ "integer", "text" ], @result.types.map { |t| t.downcase } |
There was a problem hiding this comment.
Why do we need to downcase here if it's already handled in pragmas.rb?
There was a problem hiding this comment.
@casperisfine Because the pragmas.rb change only impacts the table_info call. The types call here goes directly to the extension.
There was a problem hiding this comment.
My bad. Can we add a similar downcase in there to preserve backward compatiblity?
There was a problem hiding this comment.
@casperisfine I made that change. Please take a look and let me know what you think.
Reverted downcase in spec, and updated another spec to use a lower cased type ('BLOB' => 'blob'). Updated ordering of arguments in the latter spec.
|
|
||
| assert_equal ['hello'], row.first | ||
| assert_equal row.first.types, ['BLOB'] | ||
| assert_equal ['blob'], row.first.types |
There was a problem hiding this comment.
I think I'm confused now... Some types were capitalized before but not all of them???
There was a problem hiding this comment.
I did this a while back, so frankly I don't remember the details off the top of my head. But you can see my comments in #2 in my original description. I was seeing case inconsistency across different operating systems.
There was a problem hiding this comment.
@casperisfine You can see an action that demonstrates the issue here - https://github.com/sparklemotion/sqlite3-ruby/runs/6090043254 . Ubuntu passes (lowercase) but Windows/Mac does not. This was ultimately all coming from the underlying C code, whose behavior didn't seem to be consistent across operating systems. So I had the Ruby code case normalize, based on what had been in the specs (assuming that was the historically expected behavior)
There was a problem hiding this comment.
Ok, thanks for the info, I'll try to dig into this a bit more next week. We also need to look at what the common callers expects (e.g. Active Record)
There was a problem hiding this comment.
@casperisfine Looking at ActiveRecord's type matching, it looks like it's explicitly case insensitive - https://github.com/rails/rails/blob/74ba52ec5c5aa53e2e728d88c179b2d487fff2b4/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb#L736
There was a problem hiding this comment.
Thanks @petergoldstein, then I think we can assume this never was consistent.
|
@tenderlove / @flavorjones I suggest we merge this. |
|
@casperisfine do you need a release, too? |
|
@flavorjones not yet, I wanted this merged to fix CI on #317, for which I'd love a release yes. |
3 participants
In addition to updating the CI configuration to include Ruby 3.0 and Ruby 3.1, a number of other changes were necessary to get this to green across the board. I could use a review by the maintainers to ensure these were done correctly. They were:
mini_portilegem to themini_portile2gem, as the former isn't compatible with Ruby 3.1.table_infoto lower case, which is what the tests expect. On Mac and Windows these were returned upper case (this test failure exists in CI before this PR)With these changes, everything runs green.