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

Support offenses with no location #123

Merged

Conversation

pcallewaert
Copy link
Contributor

We noticed 2 rubocop offenses were ignored by reviewdog in our code:

  • Lint/EmptyFile
  • RSpec/SpecFilePathSuffix

These were skipped because they have no location. This meant our pull requests were green but we had some rubocop offenses if we did a full scan on the main branch.

This PR changes the behaviour by not skipping the whole offense, but only the range data.

image

Copy link
Contributor

@javierjulio javierjulio left a comment

Choose a reason for hiding this comment

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

This is a helpful change, thank you. There is a failing test though as it seems we have a test file that is blank so I believe the result is now including this offense as you expect. That will have to be updated.

@pcallewaert
Copy link
Contributor Author

The test should be fixed now :)

Copy link
Contributor

@massongit massongit left a comment

Choose a reason for hiding this comment

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

LGTM

@massongit massongit merged commit 5e23bb6 into reviewdog:master Sep 19, 2024
8 checks passed
@review-dog
Copy link
Member

Hi, @pcallewaert! We merged your PR to reviewdog! 🐶
Thank you for your contribution! ✨

We just invited you to join the @reviewdog organization on GitHub.
Accept the invite by visiting https://github.com/orgs/reviewdog/invitation.
By joining the team, you'll be a part of reviewdog community and can help the maintenance of reviewdog.

Thanks again!

Copy link
Contributor

🚀 [bumpr] Bumped! New version:v2.19.1 Changes:v2.19.0...v2.19.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants