Print the proper ::Float::INFINITY value when used as a default value

[raimo]

Summary

This will properly print ::Float::INFINITYor -::Float::INFINITY in schema.rb as the default value of a float / double precision column with value 'Infinity' or '-Infinity' (in the database adapters that support that value).

Other Information

Performance-wise, since the cast_value in the ActiveModel::Type::Float already refers to potential Infinity values, I conclude it's also appropriate to implement the corresponding ::Float::INFINITY type name print at this abstraction level, since the the schema is not printed very often (and not having this causes problems in all database adapters that support Infinity values).

Addresses #22396

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @kaspth (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[maclover7]

Can you add a regression test to confirm this fixes the problem? Thanks!!

[raimo]

I added a confirmation test for the changed parts of code. Do you also need specific regression tests?

[sgrif]

This is not the proper way to test this, please test that the schema dumper generates proper values.

[raimo]

Changed the test to use schema_dumper.

[sgrif]

Can you move it to be with the rest of the tests for this behavior?

[raimo]

Added to the schema_dumper_test.rb along with other default value tests.

[kaspth]

r? @sgrif

[raimo]

So the mysql2 adapter tests failed with the syntax of

 `float_col` float DEFAULT Infinity) ENGINE=InnoDB

which means infinity default value does not work at all in the mysql adapter, whereas in sqlite and postgres the whole chain of SQL column type printing and schema dump works. Since I don't see any tests for sqlite infinity values I'm assuming it's not inherently supported, so I'm moving the test under postgresql infinity_test.rb.

[sgrif]

It should remain in the schema dumper test, with skip unless current_adapter?(:PostgreSQL)

[raimo]

Ok. It would have been helpful, smart and saved both of our times if instead of "this" behavior you would have referred to something more specific before. Since obviously there's a specific way you want to go with, could you help a bit first to get the process forward:

Should I have a separate class from SchemaDumperDefaultsTest for the test or should it be a separate table that I'll just create in its before block (and skip both in the before block and in the actual test from others than PostgreSQL using skip unless current_adapter?(:PostgreSQL))?

[raimo]

Ok, assuming we want to go with the latter option. :)

[sgrif]

We get hundreds of these pings a day, and have limited time to deal with
them. As such we can't always reply immediately. Thank you for your patience

On Thu, Apr 21, 2016, 11:24 AM Raimo Tuisku notifications@github.com
wrote:

Ok, assuming we want to go with the latter option. :)

—
You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub
#24571 (comment)

[sgrif]

Is there any case here other than infinity and negative infinity? Could this be more explicit and be

case value
  when ::Float::INFINITY then "::Float::INFINITY"
  when -::Float::INFINITY then "-::Float::INFINITY"
  else super
end

[sgrif]

Should this handle Float::NAN as well?

[raimo]

Good idea. Done.

[raimo]

The pull request now addresses Float::NAN as well.

[sgrif]

This line makes me angry that the decision on how to interpret "NaN does not equal to NaN" should also apply to case equality