flyerhzm / rails_best_practices

a code metric tool for rails projects
http://rails-bestpractices.com
MIT License
4.16k stars 276 forks source link

False positive "unused method" with Ruby 3.1.1 #390

Closed oliverklee closed 2 years ago

oliverklee commented 2 years ago

For this controller, RBP throws an incorrect warning about find_workshop being unused in Ruby 3.1.1:

class WorkshopsController < ApplicationController
  layout 'static'

  before_action :find_workshop, only: %i[show]

  def show; end

  private
    def find_workshop
      @workshop = Workshop::Workshop.find(params.fetch(:id))
      raise ActionController::RoutingError, 'Not Found' if @workshop.draft?
    end
end

This problem does not occur with Ruby 3.0.3.

This is with rails_best_practices 1.23.1.

flyerhzm commented 2 years ago

@oliverklee I tried add workshops_controller.rb in a rails app and run rails_best_practices, but it didn't show this warning, please show me a rails repo that reproduces this issue.

oliverklee commented 2 years ago

@flyerhzm I've just given you temporary read access to our repository. You'll need to use the task/ruby-311 branch as this issue only happens with Ruby 3.1 for me, not for 3.0.

oliverklee commented 2 years ago

@flyerhzm Thanks for looking into this! :heart:

oliverklee commented 2 years ago

@flyerhzm Please let me know when you're done so I can remove you from the project again (and we can stop paying for the additional seat on the team). Thanks!

flyerhzm commented 2 years ago

@oliverklee I just cloned the repo and run rails_best_practices, I didn't see any issue.

Screen Shot 2022-03-01 at 4 58 16 PM

oliverklee commented 2 years ago

Thanks for checking! Are you using Rails 3.1? (The problem does not occur on Rails 3.0.)

You might also want to check the task/ruby-311 branch and then delete lines 27-30 in config/rails_best_practices.yml to remove the blocklist which I've added to get the CI build to pass.

flyerhzm commented 2 years ago

@oliverklee yes, I've already checked out the task/ruby-311 branch and deleted line 27-30, see

➜  justworkshops git:(task/ruby-311) ✗ ruby -v
ruby 3.1.1p18 (2022-02-18 revision 53f5fc4236) [x86_64-darwin21]
➜  justworkshops git:(task/ruby-311) ✗ cat config/rails_best_practices.yml
AddModelVirtualAttributeCheck: { }
AlwaysAddDbIndexCheck: { }
CheckSaveReturnValueCheck: { }
DefaultScopeIsEvilCheck: { }
DryBundlerInCapistranoCheck: { }
HashSyntaxCheck: { }
IsolateSeedDataCheck: { }
KeepFindersOnTheirOwnModelCheck: { }
LawOfDemeterCheck: { }
#LongLineCheck: { max_line_length: 80 }
MoveCodeIntoControllerCheck: { }
MoveCodeIntoHelperCheck: { array_count: 3 }
MoveCodeIntoModelCheck: { use_count: 2 }
MoveFinderToNamedScopeCheck: { }
MoveModelLogicIntoModelCheck: { use_count: 4 }
NeedlessDeepNestingCheck: { nested_count: 2 }
NotRescueExceptionCheck: { }
NotUseDefaultRouteCheck: { }
NotUseTimeAgoInWordsCheck: { }
OveruseRouteCustomizationsCheck: { customize_count: 3 }
ProtectMassAssignmentCheck: { }
RemoveEmptyHelpersCheck: { }
RemoveTabCheck: { }
RemoveTrailingWhitespaceCheck: { }
RemoveUnusedMethodsInControllersCheck: {
  except_methods: [
    # This is a Devise method which RBP does not know anything about.
    ApplicationController#after_sign_in_path_for,

    # This is a CanCanCan method which RBP does not know anything about.
    ApplicationController#current_ability,
    AbstractAdminController#skip_bullet,
    AbstractAdminController#strong_address_parameters
  ]
}
RemoveUnusedMethodsInHelpersCheck: { }
RemoveUnusedMethodsInModelsCheck: {
  except_methods: [
    Offer#confirm
  ]
}
ReplaceComplexCreationWithFactoryMethodCheck: { attribute_assignment_count: 2 }
ReplaceInstanceVariableWithLocalVariableCheck: { }
RestrictAutoGeneratedRoutesCheck: { }
SimplifyRenderInControllersCheck: { }
SimplifyRenderInViewsCheck: { }
UseBeforeFilterCheck: { customize_count: 2 }
UseModelAssociationCheck: { }
UseMultipartAlternativeAsContentTypeOfEmailCheck: { }
UseParenthesesInMethodDefCheck: { }
UseObserverCheck: { }
UseQueryAttributeCheck: { }
UseSayWithTimeInMigrationsCheck: { }
UseScopeAccessCheck: { }
UseTurboSprocketsRails3Check: { }
➜  justworkshops git:(task/ruby-311) ✗ bundle exec rails_best_practices .
Source Code: |======================================================================================================================================================================|

Please go to https://rails-bestpractices.com to see more useful Rails Best Practices.

No warning found. Cool!

It's still passed.

oliverklee commented 2 years ago

Now I don't get the errors anymore either on my setup. I have no idea why I got them and now don't. :confused:

Sorry for the false alarm, and a big thanks for taking the time to look into this!

Closing.

oliverklee commented 2 years ago

Very strange. We're now getting those false positives again, but only if we switch the code (Ruby and HAML) to use Ruby 3.1 syntax (particularly, omitting the hash value where has key and value are the same).