-
Notifications
You must be signed in to change notification settings - Fork 22k
Fix dirty check for Float::NAN #41663
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
🤔 I thought I've seen a similar issue/PR recently but now I can't find it. @xronos-i-am Is this still something you're interested in? |
no |
@xronos-i-am I realize you may not feel like finishing this PR, but I'd like to leave it open to see if anyone else may be interested in this. At worst, @rails-bot auto-closes this PR if there is no activity after some time. I also was wondering since I just realize this change was targeting |
There were too many issues with |
@zzak I ran the script against main and it seems that the problem still exists. I'll pick up this require "bundler/inline"
gemfile(true) do
source "https://rubygems.org"
git_source(:github) { |repo| "https://github.com/#{repo}.git" }
gem "rails", github: "rails/rails", branch: "main"
gem "sqlite3"
end
require "active_record"
require "minitest/autorun"
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Schema.define do
create_table :warehouse_receipts, force: true do |t|
t.decimal :vat_coef
end
end
class WarehouseReceipt < ActiveRecord::Base
end
class Test < Minitest::Test
def test_nan_dirty
wr = WarehouseReceipt.create!(vat_coef: Float::NAN)
wr.vat_coef = Float::NAN
assert !wr.changed? # Expected false to be truthy.
end
end |
@zzak I opened a PR with a test case |
Closing in favor of #42831 |
Summary
The problem: Float::NAN is special value in ruby. It can't be compared with
==
. Because of this dirty check failed