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

RSpec/MetadataStyle: Fix false positive for multiple strings preceding metadata #1731

Merged

Conversation

franzliedke
Copy link
Contributor

@franzliedke franzliedke commented Oct 17, 2023

This fixes a false positive in an example group like the following:

      response "404", "unauthorized access", api_docs: false do
        let(:authorization) { another_user.token }

        run_test!
      end

(This uses DSL from the rswag gem.)

Fixes #1714.


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).

@@ -46,7 +46,7 @@ def on_metadata(_symbols, _hash)

private

def on_matadata_arguments(metadata_arguments)
def on_metadata_arguments(metadata_arguments)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Drive-by typo fix 😇

# RSpec example groups accept two string arguments. In such a case,
# the rspec_metadata matcher will interpret the second string
# argument as a metadata symbol.
symbols.shift if symbols.first&.str_type?
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 first fixed this in the rspec_metadata matcher in lib/rubocop/cop/rspec/mixin/metadata.rb (see individual commits), but this made one test for the RSpec/SortMetadata cop fail. Looking at that cop, it seems to respect many more interesting cases, such as dynamic arguments, which could probably create more false positives in the MetadataStyle cop.

Example:

context "something dynamic based on", variable do
  # ...
end

I decided not to dive into these cases and leave it to the AST matcher experts. (They may also be outside the scope of this PR, as this cop didn't worry about them so far.) Feel free to amend this PR if you see an obvious improvement.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suspect the real solution in mixin/metadata.rb would be to separate which metadata can be passed to #Examples.all vs #ExampleGroups.all, but I’m not sure I want to go down that rabbit hole 😅 The suggested fix works for me, too.

@franzliedke franzliedke force-pushed the 1714-metadata-style-autocorrect-strings branch from e1e5a6e to 7464bc3 Compare October 19, 2023 20:54
@franzliedke
Copy link
Contributor Author

This should fix the tests.

# RSpec example groups accept two string arguments. In such a case,
# the rspec_metadata matcher will interpret the second string
# argument as a metadata symbol.
symbols.shift if symbols.first&.str_type?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suspect the real solution in mixin/metadata.rb would be to separate which metadata can be passed to #Examples.all vs #ExampleGroups.all, but I’m not sure I want to go down that rabbit hole 😅 The suggested fix works for me, too.

@franzliedke
Copy link
Contributor Author

@bquorning Thanks so much for the review, I appreciate it! Do I need to do something else (like squashing the commits), or just be patient? 😇

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Thank you!
I had some ideas how to improve node patterns, but will attempt separately.

@bquorning
Copy link
Collaborator

Do I need to do something else (like squashing the commits), or just be patient? 😇

Valid question 😅 If you squash the last 3 commits together, I’ll merge it.

@franzliedke franzliedke force-pushed the 1714-metadata-style-autocorrect-strings branch from 7464bc3 to 950a5b4 Compare October 27, 2023 07:34
@franzliedke
Copy link
Contributor Author

@bquorning Consider it done. 👍🏼

Copy link
Collaborator

@bquorning bquorning left a comment

Choose a reason for hiding this comment

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

Thank you 👍🏼

@bquorning bquorning merged commit 860719e into rubocop:master Oct 27, 2023
22 checks passed
@franzliedke franzliedke deleted the 1714-metadata-style-autocorrect-strings branch November 6, 2023 09:20
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.

RSpec/MetadataStyle: False positives and syntax-breaking autocorrects
3 participants