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

Update allowlist of methods to ignore for false positives #900

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

smcabrera
Copy link

@smcabrera smcabrera commented Dec 27, 2022

Created as a draft for now so I can come back and add some examples to reproduce the issue I'm having and give some more context.


Back in September of last year @pyromaniac reported an issue with false positives for the Rails/HttpPositionalArguments cop when routes are defined within specs. That issue was subsequently fixed by #533. However there was one piece that was not fully addressed.

While the following code will no longer produce a false positive:
# frozen_string_literal: true

RSpec.describe FooController do
  before do
    routes do
      get :list, on: :collection
    end

    Rails.application.routes.draw do
      get :list, on: :collection
    end
  end

  specify do
    get :list, filter: 42
  end
end
We'll still get a false positive when the `append` method is invoked
Rails.application.routes.append do
  get 'some-fake-endpoint', on: :collection

Adding to the list of methods to ignore for false positives seems to successfully address this issue. The change is very small.


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.

This PR rubocop@732a0db

This issue was reported a while back finding that the HTTP positional arguments cop could give false positives rubocop#532

I found another instance of false positives when the `append` method is used instead of `draw` or `routes`. If we add that to the list as well this problem seems to go away.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant