rubocop-rails icon indicating copy to clipboard operation
rubocop-rails copied to clipboard

Add new Rails/NoVariablePrecisionDecimal cop

Open drdee opened this issue 5 years ago • 4 comments

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.

drdee avatar Nov 15 '19 02:11 drdee

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.

drdee avatar Nov 15 '19 02:11 drdee

Hi @koic -- could you please take a look at this PR?

drdee avatar Nov 18 '19 16:11 drdee

Hi @koic -- could you please take a look at this PR?

drdee avatar Nov 28 '19 02:11 drdee

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 :-)

koic avatar Nov 30 '19 07:11 koic