Add support for custom formats in Rails/ToSWithArgument

[wata727]

This PR adds support for custom formats in Rails/ToSWithArgument cop.

Currently this cop only supports formats defined by Rails due to #869 changes, but some users may have added their own formats, such as:

Time::DATE_FORMATS[:datetime] = '%Y-%m-%d %H:%M:%S'

Even in such cases, it would be convenient if autocorrection by this cop could be used. You can add any format to FormatTypes as below.

Rails/ToSWithArgument:
  FormatTypes:
    - datetime

---

Before submitting the PR make sure the following are checked:

• The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
• Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.
• If this is a new cop, consider making a corresponding update to the Rails Style Guide.

[koic]

How does the name CustomFormatTypes sound?

[koic]

Can you add documentation text and example for the option?"

[wata727]

Thank you for your review.
Fixed https://github.com/rubocop/rubocop-rails/compare/badcd55da99b4ca769b707b0f4b1dd60c14ed564..ccc7b153e69944ba9c0f9b637d6b86c8ba39d9d0

[koic]

Hm, upon reconsideration, instead of adding a new parameter like FormatTypes, what about handling all cases where a single symbol is passed as an argument to to_s as an offense? This eliminates the need for users to make unnecessary parameter setting.

Are there any potential false positives that concern you with this approach?

NOTE: Integer argument will produce false positives. e.g., (42.to_s(8)) So, I think it's better to limit it to symbol argument.

[wata727]

I can't think of any examples, but overriding to_s is probably a common case. I believe RuboCop should be conservative unless the caller class can be identified.

Customized formatting is an undocumented behavior, and for most users there will be no need to set CustomFormatTypes. However, if you have an app like us, being able to automatically correct this will be useful when updating to Rails 7.0.

We've already taken advantage of this autocorrection to complete an update to Rails 7.0, and just wanted to open the door for users faced similar issues. If you think it's a case that RuboCop shouldn't support it, feel free to close this PR :)

[koic]

Yeah, to_s can be redefined, and this cop is already marked as unsafe:
https://docs.rubocop.org/rubocop-rails/2.22/cops_rails.html#railstoswithargument

CustomFormatTypes configuration approach is a hard-to-change interface once it's released. Time::DATE_FORMATS[:datetime] = '%Y-%m-%d %H:%M:%S' is a known technique, so it seems logical to first remove the condition as follows:

        def rails_extended_to_s?(node)
-         node.first_argument&.sym_type? && EXTENDED_FORMAT_TYPES.include?(node.first_argument.value)
+         node.first_argument&.sym_type?
        end

If there's a lot of feedback about false positives in Rails application context, it might be reasonable to add CustomFormatTypes configuration as a setting at that time, I think.

[wata727]

CustomFormatTypes configuration approach is a hard-to-change interface once it's released

Makes sense. Nonetheless, if I know in advance the type of arguments, I would like to pass format types to make auto correction more efficient.

I understand that there are concerns about adding a new config, so I'm going to withdraw this PR for now. Thank you for your review and consideration.

[silasb]

Can we reconsider this and/or add more documentation around this? I recently ran into this within a large code base and didn't realize it used the EXTENDED_FORMAT_TYPES to filter on. The documentation doesn't mention it either so the only way to find out why this wasn't adding violations was to look at the implementation. I can certainly add in a PR to fix the documentation around this.

As for the escape hatch, I liked the solution @wata727 created.