Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add ToJSON Linter #6379

Merged
merged 2 commits into from Mar 10, 2019
Merged

Add ToJSON Linter #6379

merged 2 commits into from Mar 10, 2019

Conversation

allcentury
Copy link
Contributor

@allcentury allcentury commented Oct 11, 2018 •
edited

This fixes issue #6378

classes that override to_json need to ensure an optional parameter is
available.

class MyClass
  def to_json
  end
end

While calling MyClass.new.to_json works, calling
JSON.generate(MyClass.new) results in an ArgumentError.

0.59.2 (using Parser 2.5.1.2, running on ruby 2.5.1 x86_64-linux)

Before submitting the PR make sure the following are checked:

  • 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.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run rake default. It executes all tests and RuboCop for itself, and generates the documentation.

@allcentury allcentury force-pushed the to_json_cop branch 4 times, most recently from d74163a to 2ab1725 Compare October 12, 2018 01:35
Drenmi
Drenmi reviewed Oct 12, 2018
View reviewed changes
lib/rubocop/cop/lint/to_json.rb Outdated
M_JSON

def on_def(node)
return if node.method_name != :to_json
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think these guard clauses would be clearer if combined:

return unless node.method?(:to_json) && node.arguments.empty?

This reads exactly as the condition you laid out in the description. 🙂

Drenmi
Drenmi reviewed Oct 12, 2018
View reviewed changes
lib/rubocop/cop/lint/to_json.rb Outdated

def autocorrect(node)
lambda do |corrector|
corrector.insert_after(node.loc.name, '(_opts = {})')
Copy link
Collaborator

@Drenmi Drenmi Oct 12, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think this will raise a RuboCop offense in Style/OptionHash. We should probably look at adding a whitelist entry to that cop to account for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Drenmi thanks for calling this out - where exactly would I add the whitelist?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Drenmi can you point me in the right direction?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@allcentury: The changes need to be made in Style/OptionHash. For an example of a method whitelist, check e.g. Rails/DynamicFindBy.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@allcentury: The changes need to be made in Style/OptionHash. For an example of a method whitelist, check e.g. Rails/DynamicFindBy.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@allcentury: The changes need to be made in Style/OptionHash. For an example of a method whitelist, check e.g. Rails/DynamicFindBy.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@allcentury: The changes need to be made in Style/OptionHash. For an example of a method whitelist, check e.g. Rails/DynamicFindBy.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@allcentury: The changes need to be made in Style/OptionHash. For an example of a method whitelist, check e.g. Rails/DynamicFindBy.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@allcentury: The changes need to be made in Style/OptionHash. For an example of a method whitelist, check e.g. Rails/DynamicFindBy. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Drenmi thank you! I think I've implemented your suggestion, please take a look when you have a sec

@allcentury allcentury force-pushed the to_json_cop branch 2 times, most recently from 7177f8a to 6662c89 Compare October 12, 2018 17:58
@allcentury allcentury force-pushed the to_json_cop branch 2 times, most recently from 899c9a7 to 28a443d Compare October 23, 2018 13:42
@allcentury
Copy link
Contributor Author

@Drenmi is this ready to merge?

Drenmi
Drenmi reviewed Nov 15, 2018
View reviewed changes
lib/rubocop/cop/lint/to_json.rb Outdated
# end
#
class ToJSON < Cop
MSG = <<-M_JSON.strip_indent.chomp.freeze
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's change the identifier from M_JSON (makes me think of the late king of pop 😅) to MESSAGE. 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for a heredoc here:

MSG = '`#to_json` requires an optional hash argument to be parsable ' \
  'via JSON.generate(obj).'

Drenmi
Drenmi reviewed Nov 15, 2018
View reviewed changes
spec/rubocop/cop/lint/to_json_spec.rb Outdated

it 'does not register an offense when using `#to_json` with arguments' do
expect_no_offenses(<<-RUBY.strip_indent)
def to_json(opts = {})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's indent the contents of the heredoc one more level for this and the next example. 🙂

Drenmi
Drenmi approved these changes Nov 15, 2018
View reviewed changes
Copy link
Collaborator

@Drenmi Drenmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small nits, but looks good to me. 🚀

Ping @bbatsov.

bquorning
bquorning reviewed Nov 23, 2018
View reviewed changes
lib/rubocop/cop/lint/to_json.rb Outdated
# end
#
# # good
# def to_json(opts = {})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in #6378 (comment) I am not sure the argument is always a Hash

require 'json'
class MyClass
  def to_json(*args)
    p args
    ""
  end
end
JSON.generate(MyClass.new)
#=> [#<JSON::Ext::Generator::State:0x00007faacb155a40>]

In the JSON documentation I see a few different signatures for #to_json:

…maybe we can only say that to_json should have arity 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree @bquorning - I'll make that change

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, * and *args aren’t arity 1…

[Fix rubocop#6378] Add ToJSON Linter
8fd679e 
This fixes issue rubocop#6378

classes that override to_json need to ensure an optional argument is
available.

class MyClass
  def to_json
  end
end

While calling MyClass.new.to_json works, calling
JSON.generate(MyClass.new) results in an ArgumentError.
@Drenmi
Copy link
Collaborator

Drenmi commented Jan 22, 2019

@allcentury Sorry for dropping the ball on this. Please rebase your branch on current master, ensure your CHANGELOG entry is in the right place, and I'll merge it in. Thank you! 🙇

@bbatsov bbatsov merged commit a823c57 into rubocop:master Mar 10, 2019
@mrasu mrasu mentioned this pull request Mar 22, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bquorning bquorning bquorning left review comments

@Drenmi Drenmi Drenmi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@allcentury @Drenmi @bquorning @bbatsov