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 git diff detection #2758

Merged
merged 3 commits into from
Apr 21, 2023
Merged

Fix git diff detection #2758

merged 3 commits into from
Apr 21, 2023

Conversation

Haeki
Copy link

@Haeki Haeki commented Apr 19, 2023

Pre-Request Checklist

  • Passes rubocop code analysis (try rubocop --auto-correct)

- [ ] Tests added or adapted (try rake test)
- [ ] Changes are reflected in the documentation
- [ ] User-visible changes appended to CHANGELOG.md

Description

Fixes a bug caused by the RuboCop autocorrect.
In /lib/oxidized/output/git.rb the code in line 82 caused an error.
Cause is next if commit.diff(paths: [path]).empty?, the return value of diff has no empty? function.
The old line next if commit.diff(paths: [path]).empty? was correct.
The same mistake was corrected in this commit back in 2019.
This time i also disabled the RuboCop check for this line so that autocorrect doesn't change this again.

Possibly this also solves issues 2715 and 2282 because the error when clicking the "Version" button led me to this bug. And this bugfix solved it for me.

@ewysong
Copy link

ewysong commented Apr 20, 2023

I'm on a brand new install of oxidized 0.29.0 on Ubuntu 22.04, and can confirm I had the issues mentioned in #2715 and #2282 which were solved by this pull request.

@mikegleasonjr
Copy link

mikegleasonjr commented Apr 20, 2023

Fixed for me too

@codecov-commenter
Copy link

Codecov Report

Patch and project coverage have no change.

Comparison is base (637434b) 66.40% compared to head (431aa79) 66.40%.

❗ Current head 431aa79 differs from pull request most recent head 511effa. Consider uploading reports for the commit 511effa to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2758   +/-   ##
=======================================
  Coverage   66.40%   66.40%           
=======================================
  Files          30       30           
  Lines        1500     1500           
=======================================
  Hits          996      996           
  Misses        504      504           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@aschaber1
Copy link
Collaborator

I rewrote this into an inline disablement, as it's used more commonly elsewhere in the repo instead of the block, when just a single line is marked.

Thank you for your help :)

@aschaber1 aschaber1 merged commit 40e8dab into ytti:master Apr 21, 2023
@Haeki Haeki deleted the fix-git-diff-detection branch September 6, 2023 12:53
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.

7 participants