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

Fix RSpec/FilePath when checking an empty class #1000

Merged
merged 1 commit into from Aug 17, 2020
Merged

Conversation

@bquorning
Copy link
Member

@bquorning bquorning commented Aug 17, 2020

Remember that RSpec/FilePath checks all files - not only **/*_spec.rb. And when some class defines e.g. an empty class, the TopLevelGroup module would raise an error.

Fixes #999.


Before submitting the PR make sure the following are checked:

  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • [-] Updated documentation.
  • Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
  • The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).
Remember that `RSpec/FilePath` checks *all* files - not only
**/*_spec.rb. And when some class defines e.g. an empty class, the
`TopLevelGroup` module would raise an error.

Fixes #999.
@bquorning bquorning requested a review from pirj Aug 17, 2020
@bquorning bquorning changed the title Fix `RSpec/FilePath` when checking an empty class Fix RSpec/FilePath when checking an empty class Aug 17, 2020
@bquorning
Copy link
Member Author

@bquorning bquorning commented Aug 17, 2020

Congratulations on the 1K pull requests (and issues) 🎉

@bquorning
Copy link
Member Author

@bquorning bquorning commented Aug 17, 2020

I hope it’s OK to ignore Code Climate’s report on added complexity…

@bquorning bquorning requested a review from Darhazer Aug 17, 2020
Copy link
Member

@pirj pirj left a comment

We have spec/smoke_tests/empty_spec.rb. How did it happen to pass?

@@ -204,6 +204,14 @@
RUBY
end

# RSpec/FilePath runs on all files - not only **/*_spec.rb

This comment has been minimized.

@pirj

pirj Aug 17, 2020
Member

Wondering, why so?
For some reason relevant_rubocop_rspec_file? evaluates to true for it.

This comment has been minimized.

@pirj

pirj Aug 17, 2020
Member

Ah, I see it

        def relevant_rubocop_rspec_file?(_file)
          true
        end
@@ -38,7 +38,9 @@ def top_level_group?(node)
end

def top_level_nodes(node)
if node.begin_type?
if node.nil?
[]

This comment has been minimized.

@pirj

pirj Aug 17, 2020
Member

I remember we were discussing this with Maxim and settled on

return unless root_node

Do you think both checks are necessary?

This comment has been minimized.

@bquorning

bquorning Aug 17, 2020
Author Member

As you mention, we already have return unless root_node inside #on_new_investigation. The problem happens when the root node is an empty module or class:

elsif node.module_type? || node.class_type?
  top_level_nodes(node.body)

This comment has been minimized.

@bquorning

bquorning Aug 17, 2020
Author Member

I mean, we could move the check into the elsif body:

elsif node.module_type? || node.class_type?
  node.body.nil? ? [] : top_level_nodes(node.body)

or

elsif node.module_type? || node.class_type?
  if node.body
    top_level_nodes(node.body)
  else
    []
  end
@pirj
Copy link
Member

@pirj pirj commented Aug 17, 2020

Congratulations on the 1K pull requests (and issues)

🎉 image

@pirj
pirj approved these changes Aug 17, 2020
Copy link
Member

@pirj pirj left a comment

Look good!

@bquorning bquorning merged commit 96495ab into master Aug 17, 2020
12 of 13 checks passed
12 of 13 checks passed
codeclimate 1 issue to fix
Details
ci/circleci: code-climate Your tests passed on CircleCI!
Details
ci/circleci: confirm_config_and_documentation Your tests passed on CircleCI!
Details
ci/circleci: edge-rubocop Your tests passed on CircleCI!
Details
ci/circleci: jruby Your tests passed on CircleCI!
Details
ci/circleci: rspec-2.4 Your tests passed on CircleCI!
Details
ci/circleci: rspec-2.5 Your tests passed on CircleCI!
Details
ci/circleci: rspec-2.6 Your tests passed on CircleCI!
Details
ci/circleci: rspec-2.7 Your tests passed on CircleCI!
Details
ci/circleci: rubocop-2.4 Your tests passed on CircleCI!
Details
ci/circleci: rubocop-2.5 Your tests passed on CircleCI!
Details
ci/circleci: rubocop-2.6 Your tests passed on CircleCI!
Details
ci/circleci: rubocop-2.7 Your tests passed on CircleCI!
Details
@bquorning bquorning deleted the fix-file-path-bug branch Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.