add cop for standardizing json serialization to to_json #12314

exterm · 2023-10-25T17:25:03Z

Ruby codebases often use JSON.generate and .to_json interchangeably. The Style/ToJson cop helps unify to to_json to reduce confusion, and includes a safe (I hope) autocorrect.

# bad
JSON.generate(something)

# good
something.to_json

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.

sambostock

👋 Hey @exterm!

lib/rubocop/cop/style/to_json.rb

sambostock · 2023-10-30T03:09:31Z

lib/rubocop/cop/style/to_json.rb

+          return unless json_generate?(node)
+
+          add_offense(node) do |corrector|
+            _receiver, _method_name, *args = *node


I could swear there's an InternalAffairs cop that's supposed to disallow deducting nodes (in favor of using the accessors), and I suspect you just found a bug in it (using * splat on the right hand side).

Found the cop I was thinking of.

What's weird is that it should catch this, based on this spec.

What's even weirder is that if I add this exact file as an example in node_destructuring_spec.rb, and expect the offense, the spec passes.

diff --git a/spec/rubocop/cop/internal_affairs/node_destructuring_spec.rb b/spec/rubocop/cop/internal_affairs/node_destructuring_spec.rb index 72236e565..3a6ae8f87 100644 --- a/spec/rubocop/cop/internal_affairs/node_destructuring_spec.rb +++ b/spec/rubocop/cop/internal_affairs/node_destructuring_spec.rb @@ -29,4 +29,67 @@ RSpec.describe RuboCop::Cop::InternalAffairs::NodeDestructuring, :config do lhs, rhs = array.children RUBY end + + context 'example from #12314' do + it 'registers an offense' do + expect_offense(<<~'RUBY') + # frozen_string_literal: true + + module RuboCop + ... + add_offense(node) do |corrector| + _receiver, _method_name, *args = *node + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use the methods provided with the node extensions instead of manually destructuring nodes. + data, opts = handle_args(args) + ... + end + RUBY + end + end end

cc. @koic Any idea why CI didn't catch this?

lib/rubocop/cop/style/to_json.rb

exterm · 2023-10-30T13:24:04Z

I don't have a ton of experience writing cops, so thank you for your suggestions @sambostock , these are super helpful.

spec/rubocop/cop/style/to_json_spec.rb

lib/rubocop/cop/style/to_json.rb

spec/rubocop/cop/style/to_json_spec.rb

exterm · 2023-11-04T17:38:02Z

Ready for re-review, @sambostock

sambostock · 2023-11-04T19:02:39Z

spec/rubocop/cop/style/to_json_spec.rb

+    RUBY
+  end
+
+  it 'ignores calls to JSON.generate with unexpected arguments' do


Unlikely edge case, but should we check passing a block?

JSON.generate(foo) { nil }

What behavior would we expect? I think I'm OK with either the cop ignoring this or transforming to foo.to_json { nil }.

On the other hand, if that code is valid, it probably means that someone has overridden JSON.generate, in which case we shouldn't transform it...?

Not sure it's worth investing time in this edge case, but I will if you think it is.

What behavior would we expect?

Probably just ignoring it, but yeah, it's a crazy edge case, so it's probably fine to consider out of scope.

sambostock · 2023-11-04T19:04:55Z

config/default.yml

@@ -5379,6 +5379,11 @@ Style/TernaryParentheses:
    - require_parentheses_when_complex
  AllowSafeAssignment: true

+Style/ToJson:
+  Description: 'Standardize on `to_json` over `JSON.generate`.'
+  Enabled: false


Do we mean for this to be disabled by default, or do we think this is something the community could agree on (and therefore should be pending, to be enabled by default in the next major release, IIRC)?

I'd rather not become blocked on the inevitable bikeshed 😆
But open to opinions...

Fair. It can always be changed from false to pending in a follow up PR.

sambostock · 2023-11-04T21:57:02Z

Oh, and I think you'll want to squash this into a single commit matching the changelog entry, and ping @koic for review.

exterm · 2023-11-05T03:07:24Z

Yeah, I kept commits separate for now to make it easier to follow along with the changes I'm making based on your review. I'll squash once we're done iterating.

exterm · 2023-11-09T22:24:23Z

Squashed. This is ready for final review @sambostock @koic

sambostock

✅ LGTM, but I'm not a maintainer, so I can't approve 😅

cc. @koic

sambostock · 2023-11-09T22:47:07Z

config/default.yml

@@ -5379,6 +5379,11 @@ Style/TernaryParentheses:
    - require_parentheses_when_complex
  AllowSafeAssignment: true

+Style/ToJson:
+  Description: 'Standardize on `to_json` over `JSON.generate`.'
+  Enabled: false


Fair. It can always be changed from false to pending in a follow up PR.

sambostock · 2023-11-09T22:49:04Z

spec/rubocop/cop/style/to_json_spec.rb

+    expect_offense(<<~RUBY)
+      ::JSON.generate(foo)
+      ^^^^^^^^^^^^^^^^^^^^ Use `.to_json` instead of `JSON.generate`.
+    RUBY


I notice this doesn't have an expect_correction. Probably not a huge deal, since the case above basically covers it, but I thought I'd mention it, since I can't recall seeing specs that don't also expect the correction.

sambostock · 2023-11-09T22:53:02Z

spec/rubocop/cop/style/to_json_spec.rb

+    RUBY
+  end
+
+  it 'ignores calls to JSON.generate with unexpected arguments' do


What behavior would we expect?

Probably just ignoring it, but yeah, it's a crazy edge case, so it's probably fine to consider out of scope.

koic · 2023-11-16T10:33:44Z

The context provided in your explanation is clear, so it appears to be a style unification for ActiveModel::Serializer. Since to_json is not a method by Ruby's core but rather context-specific, it might be more appropriate to place it under the Rails department. Can you reopen this PR as a cop under rubocop-rails? I will review this one there.
https://github.com/rubocop/rubocop-rails

sambostock · 2023-11-16T15:33:38Z

@koic to_json appears to be provided by the json gem that ships with Ruby; Rails just extends that support to classes such as ActiveRecord::Base.

koic · 2023-11-16T15:51:44Z

Yes, I understand that, and I am referring to the context in which this style should be enforced.

exterm · 2023-11-17T00:00:05Z

I had thought about that, but I decided that rubocop core is more appropriate.

I may be missing something, but it seems that in all Ruby code we have the choice between JSON.generate and .to_json. It makes sense to standardize on one of them.

The only reason why I brought up Rails is that in Rails applications we can't (at least not easily) standardize on JSON.generate, so the version that makes more sense there is to standardize on .to_json.

Why would this not make sense for non-Rails Ruby code?

koic · 2023-11-17T05:27:37Z

Users might intentionally use JSON.generate, and it's not clear why to_json always should be preferred in this case. However, within the context of Rails, there seem to be distinct advantages to using to_json. This is why it appears more appropriate for rubocop-rails. In that sense, whether to_json being "standardized" is a universally accepted view in the Ruby community is not entirely clear. For instance, if the Ruby Style Guide explicitly states a rule preferring to_json, that could serve as a basis for its inclusion in the Style department of RuboCop. However, I'm not sure if in such a case, uniformity should take precedence over the user's intention.

exterm · 2023-11-19T03:50:25Z

Okay! I think I may have misunderstood the semantics of including a cop in Rubocop core. I didn't intent to enable this one by default and thus establish a new part of the style guide. I intended this cop to provide a choice, so that users / teams / projects that want to standardize on to_json can do so, and everyone else is unaffected.

However, it seems that doesn't fit with established semantics, so I'm happy to move it over.

sambostock · 2023-11-19T06:42:06Z

I still think this cop could be valuable to projects not using Rails, as one may still want to standardize on one or the other, and to_json is a non-Rails API.

I notice that there's precedent for rubocop-rails changing the config of non-Rails/... cops.

@koic, what do you think of providing this cop in rubocop, but keeping it disabled by default and then enabling it by default in rubocop-rails?

# rubocop/config/default.yml
Style/ToJson:
  Enabled: false

# rubocop-rails/config/default.yml
Style/ToJson:
  Enabled: true

This would make the cop available to the entire Ruby community, and allow us to standardize on the to_json convention in Rails projects.

lib/rubocop/cop/style/to_json.rb

exterm · 2023-12-01T02:12:02Z

Any additional thoughts, @koic ?

exterm force-pushed the pm/add-to_json-cop branch 2 times, most recently from 16f60b8 to 1d4a709 Compare October 25, 2023 18:51

exterm marked this pull request as ready for review October 25, 2023 18:55

sambostock reviewed Oct 30, 2023

View reviewed changes

spec/rubocop/cop/style/to_json_spec.rb Outdated Show resolved Hide resolved

lib/rubocop/cop/style/to_json.rb Show resolved Hide resolved

lib/rubocop/cop/style/to_json.rb Outdated Show resolved Hide resolved

spec/rubocop/cop/style/to_json_spec.rb Show resolved Hide resolved

exterm force-pushed the pm/add-to_json-cop branch from 1d4a709 to 9075e66 Compare November 4, 2023 17:25

sambostock reviewed Nov 4, 2023

View reviewed changes

exterm force-pushed the pm/add-to_json-cop branch from fc51d93 to 862aeb9 Compare November 9, 2023 22:23

sambostock reviewed Nov 9, 2023

View reviewed changes

exterm commented Nov 30, 2023

View reviewed changes

lib/rubocop/cop/style/to_json.rb Outdated Show resolved Hide resolved

add cop for standardizing json serialization to to_json

da1c1f1

exterm force-pushed the pm/add-to_json-cop branch from 862aeb9 to da1c1f1 Compare December 1, 2023 02:11

bbatsov assigned koic Mar 10, 2024

bbatsov requested a review from koic April 8, 2024 06:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add cop for standardizing json serialization to to_json #12314

add cop for standardizing json serialization to to_json #12314

exterm commented Oct 25, 2023 •

edited

sambostock left a comment

sambostock Oct 30, 2023

sambostock Oct 30, 2023

sambostock Oct 30, 2023

exterm commented Oct 30, 2023

exterm commented Nov 4, 2023

sambostock Nov 4, 2023

exterm Nov 5, 2023 •

edited

exterm Nov 5, 2023

sambostock Nov 9, 2023

sambostock Nov 4, 2023

exterm Nov 9, 2023 •

edited

sambostock Nov 9, 2023

sambostock commented Nov 4, 2023 •

edited

exterm commented Nov 5, 2023

exterm commented Nov 9, 2023

sambostock left a comment

sambostock Nov 9, 2023

sambostock Nov 9, 2023

sambostock Nov 9, 2023

koic commented Nov 16, 2023

sambostock commented Nov 16, 2023

koic commented Nov 16, 2023

exterm commented Nov 17, 2023

koic commented Nov 17, 2023 •

edited

exterm commented Nov 19, 2023 •

edited

sambostock commented Nov 19, 2023

exterm commented Dec 1, 2023

add cop for standardizing json serialization to to_json #12314

Are you sure you want to change the base?

add cop for standardizing json serialization to to_json #12314

Conversation

exterm commented Oct 25, 2023 • edited

sambostock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exterm commented Oct 30, 2023

exterm commented Nov 4, 2023

Choose a reason for hiding this comment

exterm Nov 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exterm Nov 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sambostock commented Nov 4, 2023 • edited

exterm commented Nov 5, 2023

exterm commented Nov 9, 2023

sambostock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koic commented Nov 16, 2023

sambostock commented Nov 16, 2023

koic commented Nov 16, 2023

exterm commented Nov 17, 2023

koic commented Nov 17, 2023 • edited

exterm commented Nov 19, 2023 • edited

sambostock commented Nov 19, 2023

exterm commented Dec 1, 2023

exterm commented Oct 25, 2023 •

edited

exterm Nov 5, 2023 •

edited

exterm Nov 9, 2023 •

edited

sambostock commented Nov 4, 2023 •

edited

koic commented Nov 17, 2023 •

edited

exterm commented Nov 19, 2023 •

edited