Support Rails 6.1 ActiveModel::Errors #62
Conversation
|
Coverage fails expectedly, because one of the branches in |
rewritten
force-pushed
the
activemodel-errors-rails-6-1
branch
from
6e7d044 to
6f90086
Compare
|
Do you need to keep compatibility with ruby 2.4? |
rewritten
force-pushed
the
activemodel-errors-rails-6-1
branch
from
4789e97 to
b76443c
Compare
ozeron
requested review from
andriusch and
ojab
and removed request for
andriusch
rewritten
force-pushed
the
activemodel-errors-rails-6-1
branch
from
b76443c to
145ac1a
Compare
rewritten
force-pushed
the
activemodel-errors-rails-6-1
branch
from
65834ea to
051a41d
Compare
| if ActiveModel.version < Gem::Version.new('6.1.0') | ||
| def merge_errors(other_errors) | ||
| errors.messages.deep_merge!(other_errors.messages) do |_, this, other| | ||
| (this + other).uniq | ||
| end | ||
| end | ||
| else | ||
| def merge_errors(other_errors) | ||
| other_errors.each do |error| | ||
| errors.import(error) unless errors.added?(error.attribute, error) | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
This branching is the reason coverage has decreased. Only one of the two branches will ever be used in a single test run.
Anyway, the tests run in ruby 2.4 use activemodel 5, so the legacy execution branch is covered.
| @@ -43,13 +43,13 @@ def execute_perform!(*) | |||
| specify do | |||
| expect do | |||
| expect { action.perform! }.to raise_validation_error.on_attribute(:raise_error) | |||
| end.to fail_with('expected to raise validation error on attribute :raise_error, but raised {:base=>[{:error=>:some_error}], :raise_error=>[]}') | |||
| end.to fail_with(include 'expected to raise validation error on attribute :raise_error, but raised {:base=>[{:error=>:some_error}]') | |||
There was a problem hiding this comment.
The extra reported error was an artifact due to older activemodel initializing a message array on access (and our usage of deep_merge!). Test is changed to only check the relevant part of the message.
There was a problem hiding this comment.
@rewritten Code LGTM. Could you please clarify the gemspec / Gemfile question? Thanks 😄
Gemfile
Outdated
|
|
||
| gem 'active_data', github: 'toptal/active_data', branch: 'master' | ||
|
|
||
| gem 'activerecord', '>= 5.0', '< 7' |
There was a problem hiding this comment.
@rewritten Why this change?
Having dev dependencies in gemspec has the benefit of being compatible with the rest of the ecosystem. E.g. Rubygems maps reverse deps for a gem, by analysing gemspec files.
Unless there is a valid reason for this, I would ask you to revert this change. Thanks. 😄
There was a problem hiding this comment.
Ok. But rubygems never uses development dependencies when resolving transitive deps. I don't know where else the development dependencies are used (except for rubygems website).
There was a problem hiding this comment.
For reference, Marc-André answer to the same topic in What are the practical advantages of using add_development_dependency in .gemspec?
It's not clear there is any.
In theory, RubyGems has the information necessary to run tests with add_development_dependency and test_files (see this question).
Some believe that this should be outside the scope of RubyGems (see this huge thread).
In practice, there is currently no real advantage, and the fact that RubyGems still installs test files by default is a drawback, as might be the lack of flexibility that Gemfile offers.
There was a problem hiding this comment.
@rewritten Thanks for all the patience. You did an excellent job. 👍
| @@ -13,12 +13,12 @@ Gem::Specification.new do |s| | |||
| s.files = `git ls-files`.split("\n").grep(/\A(app|lib|config|LICENSE)/) | |||
| s.license = 'MIT' | |||
|
|
|||
| s.add_runtime_dependency 'actionpack', '>= 5.1', '< 6.1' | |||
| s.add_runtime_dependency 'actionpack', '>= 5.1', '< 7' | |||
There was a problem hiding this comment.
Looks bad without check for rails+active_data versions :/
Right now granite+rails-6.1 would just fail in unexpected places without knowledge that active_data from some-non-upstream git repo should be used.
ActiveModel::Errors class has been revamped and it is now not compatible with most of the existing (and already deprecated) idioms (adding errors with
record.errors[:attr] << 'My error', mutating the hash returned by#messages, and a big chunk of the remaining API is deprecated.This change aims to support both pre-6.1 and post 6.1 versions with as little conditionals as possible.
It also includes a patch to ActiveData's nested validator, which attempts to mutate directly the error hash (which is not a hash anymore). A similar patch is available at pyromaniac/active_data#76.
Review
Pre-merge checklist
master(if not - rebase it).