rubocop-rails
rubocop-rails copied to clipboard
Add new Rails/NoVariablePrecisionDecimal cop
This PR introduces a cop to enforce the explicit specification of both the precision
and scale
parameters for a Decimal column. Data stored in a column without these two parameters will result in variable length precision which makes downstreaming processing a lot harder, particularly when these data is imported into a data warehouse (Redshift, Snowflake, etc etc).
Before submitting the PR make sure the following are checked:
- [x] Wrote good commit messages.
- [ ] Commit message starts with
[Fix #issue-number]
(if the related issue exists). - [x] Feature branch is up-to-date with
master
(if not - rebase it). - [x] Squashed related commits together.
- [x] Added tests.
- [x] Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
- [x] The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
- [x] Run
bundle exec rake default
. It executes all tests and RuboCop for itself, and generates the documentation.
circle-ci is failing because of lib/rubocop/cop/rails/rake_environment.rb:49:13: C: Style/RedundantReturn: Redundant return detected. return first_arg.value.to_sym ^^^^^^
but I don't believe that failure is related to this PR.
Hi @koic -- could you please take a look at this PR?
Hi @koic -- could you please take a look at this PR?
The type returned by Active Record depends on a value specified for scale
as follows.
if scale == 0
# FIXME: Remove this class as well
Type::DecimalWithoutScale.new(precision: precision)
else
Type::Decimal.new(precision: precision, scale: scale)
end
https://github.com/rails/rails/blob/v6.0.2.rc1/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb#L656-L661
And Active Record wants to remove Type::DecimalWithoutScale
as written in the source code comment.
For this reason, it seems that the Cop name is Rails/DecimalWithoutScale
cop.
I think an example code can be as follows.
#
# @example
#
# # bad
# add_column :fees, :amount, :decimal
# add_column :fees, :amount, :decimal, scale: 0 # scale is 0
#
# # good
# add_column :fees, :amount, :decimal, scale: 4
# add_column :fees, :amount, :decimal, precision: 6, scale: 4
#
# @example RequirePrecision: false (default)
#
# # good
# add_column :fees, :amount, :decimal, scale: 4
#
# @example RequirePrecision: true
#
# # bad
# add_column :fees, :amount, :decimal, scale: 4
#
I think that the omission due to unspecified scale
is a problem as a property of using decimal
type. On the other hand, users may want to rely on adapter-specific default values for "precision" per RDBMS. In the first release I'd like to try the default value of the RequirePrecision
option with false.
Finally, the same can be said for t.decimal
, so I think this cop can support it as well :-)