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.

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

Rails/Output should not allow "puts" (without argument) #934

Closed
stoivo opened this issue Feb 9, 2023 · 2 comments · Fixed by #965
Closed

Rails/Output should not allow "puts" (without argument) #934

stoivo opened this issue Feb 9, 2023 · 2 comments · Fixed by #965
Labels
bug Something isn't working

Comments

@stoivo
Copy link
Contributor

stoivo commented Feb 9, 2023

Expected behavior

I recently updated ruby and rails, not sure what cased me to find this but I think it is a bug anyway. Rails/Output don't like this code below in my migration, which is correct. I think I agree with #175 that this cop should ignore migrations by default but thats a different issue

    puts
    puts "WARNING: Partially irreversible migration!"
    puts
    puts "It is not possible to reconstruct the data deleted in this migration."
    puts "Reverted the structure, but not the data"
    puts

The issue is that the suggestions is this. And this code is accepted.

    puts
    Rails.logger.debug "WARNING: Partially irreversible migration!"
    puts
    Rails.logger.debug "It is not possible to reconstruct the data deleted in this migration."
    Rails.logger.debug "Reverted the structure, but not the data"
    puts

I think this is what should happend

    Rails.logger.debug
    Rails.logger.debug "WARNING: Partially irreversible migration!"
    Rails.logger.debug
    Rails.logger.debug "It is not possible to reconstruct the data deleted in this migration."
    Rails.logger.debug "Reverted the structure, but not the data"
    Rails.logger.debug

Actual behavior

This code is accepted but should be

    puts
    Rails.logger.debug "WARNING: Partially irreversible migration!"
    puts
    Rails.logger.debug "It is not possible to reconstruct the data deleted in this migration."
    Rails.logger.debug "Reverted the structure, but not the data"
    puts

Steps to reproduce the problem

I coded the repo from master at sha fd69f79 and wrote a failing spec. It might not be the correct place for this

diff --git a/spec/rubocop/cop/rails/output_spec.rb b/spec/rubocop/cop/rails/output_spec.rb
index ce501f840..d11548b69 100644
--- a/spec/rubocop/cop/rails/output_spec.rb
+++ b/spec/rubocop/cop/rails/output_spec.rb
@@ -23,6 +23,17 @@ RSpec.describe RuboCop::Cop::Rails::Output, :config do
     RUBY
   end
 
+  it 'registers and corrects an offense for using `puts` method without a receiver' do
+    expect_offense(<<~RUBY)
+      puts
+      ^^^^ Do not write to stdout. Use Rails's logger if you want to log.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      Rails.logger.debug
+    RUBY
+  end
+
   it 'registers and corrects an offense for using `print` method without a receiver' do
     expect_offense(<<~RUBY)
       print "abbe busoni"

RuboCop version

cloned at fd69f79 and had a failing spec

@fatkodima
Copy link
Contributor

Makes sense to me.
There is a test case for this behavior -

it 'does not record an offense for methods without arguments' do
expect_no_offenses(<<~RUBY)
print
pp
puts
$stdout.write
STDERR.write
RUBY
end

@koic Do you have an idea why is that?

@stoivo
Copy link
Contributor Author

stoivo commented Feb 11, 2023

The test has added in the initial cop creation 9f70ec7

@koic koic added the bug Something isn't working label Mar 29, 2023
koic added a commit to koic/rubocop-rails that referenced this issue Mar 29, 2023
Fixes rubocop#934.

This PR fixes a false negative for `Rails/Output`
when print methods without arguments.

rubocop/rubocop#1184 reported the following problem,
but it seems that it was mistakenly updated to allow no argument cases.

```console
expect(p.arity).to be 1
       ^
spec/models/cco/blog_cco2_spec.rb:46:16: C: Do not write to stdout. Use Rails' logger if you want to log.
```

It seems that rubocop/rubocop#6829 has since used that mistake.
So this PR steers `Rails/Output` back to its original intent.
koic added a commit to koic/rubocop-rails that referenced this issue Mar 29, 2023
Fixes rubocop#934.

This PR fixes a false negative for `Rails/Output`
when print methods without arguments.

rubocop/rubocop#1184 reported the following problem,
but it seems that it was mistakenly updated to allow no argument cases.

```console
expect(p.arity).to be 1
       ^
spec/models/cco/blog_cco2_spec.rb:46:16: C: Do not write to stdout. Use Rails' logger if you want to log.
```

It seems that rubocop/rubocop#6829 has since used that mistake.
So this PR steers `Rails/Output` back to its original intent.
@koic koic closed this as completed in #965 Mar 30, 2023
koic added a commit that referenced this issue Mar 30, 2023
[Fix #934] Fix a false negative for `Rails/Output`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants