support rails 7.1.0 #382
support rails 7.1.0 #382
Conversation
|
There are more changes to do. I will open prs in the other repos. |
|
Just looked at the code now. I am really unsure about adding rails dotenv for all developers. Is it necessary for rails 7.1 dev or is it for your comfort? I personally use a global dotenv tool that is agnostic to language or project! A solution could be like they do in rails gemfile: load a gemfile.local if it exists, then you could use this without having this for everyone! Besides that, I'm curious about what change you are mentioning @seuros and definitely down for other PRs if they're necessary. |
|
@BuonOmo the dot env is added to this gem testing, it not a requirement in runtime. It only activate if you have a .env in the gem folder . (i use it to connect to remote databases by setting PG*) |
|
Yes I know this gem no worries. I just think that if it is for your own comfort it shouldn't be part of that PR :) FYI I use https://direnv.net/ to avoid having this per project setting. |
|
I will remove it. I did not want to ship a green pr that was falling in AR behaviors, the new ar has change in the api signature. Thanks for the direnv. I started using zsh dotenv plugin. |
|
@BuonOmo Ready ! |
activerecord-postgis-adapter.gemspec
Outdated
| @@ -19,13 +19,13 @@ Gem::Specification.new do |spec| | |||
|
|
|||
| spec.required_ruby_version = ">= 2.7.0" | |||
|
|
|||
| spec.add_dependency "activerecord", "~> 7.0.0" | |||
| spec.add_dependency "activerecord", "~> 7.1.beta1" | |||
There was a problem hiding this comment.
| spec.add_dependency "activerecord", "~> 7.1.beta1" | |
| spec.add_dependency "activerecord", ">= 7.0", "< 7.2" |
we still do support AR 7.0.8 don't we? @keithdoggett I don't know how we handled that in the past but it seems to me that we are quite strict, aren't we?
There was a problem hiding this comment.
We bumped major version at every AR major version. It on the readme.
Also API are not compatible fully.
There was a problem hiding this comment.
Two things though:
- this is not a major version
- shouldn't we care about compatibility for our users ?
There was a problem hiding this comment.
In rails, that a major version .
7.0 to 7.1 is major since it has breaking changes.
There was a problem hiding this comment.
The user api in this adapter is unchanged.
Users can updated it freely and the others small changes were just throwing warnings in ruby 3.2
lib/active_record/connection_adapters/postgis/spatial_table_definition.rb
Show resolved
Hide resolved
|
Hi @seuros sorry for the abundancy of reviews, but TBH I feel like the PR could be way simpler, focusing on the actual goal of supporting the 7.1... These other changes are making it quite confusing to me. Anyway, I think we shouldn't be so far from merging. Although I'd love your opinion @keithdoggett as you have done these kind of changes in the past! |
|
Thanks for the review @BuonOmo! Even though this is a minor version bump in Rails/ActiveRecord since it introduces breaking changes to the internals we need to have a separate major version of this gem to support it. I can't remember the last time we were able to support a minor bump in rails with just a minor bump in this gem (it may predate my time maintaining it), but it's been a while. @seuros I need to create a few other branches for old version but will be able to merge this soon. Thanks for all the work on this! |
|
Gotcha, thanks for the explanation. Shouldn't we wait for the stable 7.1 then ? Or name ours 9.0.0.beta (or rc2 as the rails rc2 is out). Just to be sure we won't need another major for that ! |
|
7.1 is out today. It works. |
|
Don't you want to merge it with the latest AR release candidate? Otherwise lgtm |
This add support to rails 7.1.0