rrrene
Diff is checking files completely
Environment
- Credo version (
mix credo -v): 1.6.1 - Erlang/Elixir version (
elixir -v): Erlang/OTP 23 - Elixir 1.11.2 (compiled with Erlang/OTP 21) - Operating system: macOS Monterey
What were you trying to do?
We are trying to get the list of new issues introduced on the changes of a given branch compared to the merge base.
We are using this command:
$ mix credo diff --from-git-merge-base origin/master
Expected outcome
After reading the documentation of this feature, we were expecting to receive only the issues introduced on those code changes
Actual outcome
Credo was returning all issues on the files changed on that branch, without checking if they were introduced on that branch or not.
Background
Our expected outcome may be a bit confusing, but I'm working on a software which hasn't Credo before and I want to introduce it gradually to avoid massive conflicts with other contributions.
I'm not sure which behavior is the expected one, or if the one I expected was even feasible at all, as code changes may introduce some new issues and move other (if we look at line number as the source for this).
Thx for reprting!
Credo was returning all issues on the files changed on that branch, without checking if they were introduced on that branch or not.
Could you describe this in more in more detail or point me to an open source repo where this happens?
Hi! Thanks for answering to this issue.
The issue was like this:
- We have several big (really big) files with a lot of credo issues, mostly related to not being formatted (we are working on that, but as in any big project with lot of committers, we cannot make a big bang change).
- Someone changed a function in the module, not specially at the beginning or the bottom.
- Credo diff reported all issues on the file, not only in the changed lines.
As the file was super big, probably git is messing things with lines changed or some nasty bug happened in credo.
The problem is that the project itself is closed source, and I cannot share the exact example. I have tried making a separate repo but I couldn't reproduce it (but we had this issue happening for more than a week on this repo).
As I could not reproduce it in a way I can share it with you, and it looks like is not a bug happening to anyone else, feel free to close this.
Past week we have been working on enabling each check in separate PRs fixing the issues on the code, so we are currently in a position in which we don't need the diff functionality anymore - however, thanks for taking time on this.
I'm running into the same issue with a number of checks though I could be doing something wrong.
Example repo: https://github.com/bradleygolden/credo-diff-test
The main branch includes an alias and import with invalid ordering. The test branch includes a change to add Credo.Check.Readability.StrictModuleLayout. This is the output I get on that branch when running credo:
➜ credo_diff_test git:(test) mix credo diff --from-git-merge-base origin/main
Diffing 6 source files in working dir with origin/main ...
Code Readability
┃
+ ┃ [R] ↘ import must appear before alias
+ ┃ apps/example/lib/example.ex:17:3 #(Example)
Please report incorrect results: https://github.com/rrrene/credo/issues
Analysis took 0.06 seconds (0.06s to load, 0.00s running 1 check on 6 files)
Changes between 7c8b168b088cf506d4545ff4f334fa95096c7456 and working dir:
+ added 1 new code readability issue,
✔ fixed no issues, and
~ kept no issues.
Use `mix credo explain` to explain issues, `mix credo diff --help` for options.
Because the change already exists on main, I wouldn't expect anything to be reported but it still is. Any thoughts on why this would be happening?
Okay I resolved the issue above by branching off of the test branch (branch-off-test). Running credo diff on this branch worked as expected:
➜ credo_diff_test git:(branch-off-test) mix credo diff --from-git-merge-base origin/test
Diffing 7 source files in working dir with origin/test ...
Please report incorrect results: https://github.com/rrrene/credo/issues
Analysis took 0.1 seconds (0.1s to load, 0.00s running 1 check on 7 files)
Changes between 3179260c0ab39dcd645237cb515b8ae966afff81 and working dir:
+ added no issues,
✔ fixed 1 code readability issue, and
~ kept 1 code readability issue.
Use `mix credo explain` to explain issues, `mix credo diff --help` for options.
The intent of this change was to test that the actual diff functionality works as expected once credo rules are committed on the base branch.
The issue that I'm having is that I want to add new rules but because those rules won't work properly with diffs until they're committed and merged, I'd have to forcefully merge them even though my checks are failing. Ideally I'd be able to add new checks in a branch and those checks would be evaluated against diffs only.
I apologize for the age/inactivity on this issue. I should have done a better job at resolving this properly. 😥
If I understand the problem correctly, we would have to track changes to files and for all found issue, we would have to identify whether or not the function/module was changed by the edits.
I am not sure that there is an obvious solution to this that I am just not seeing.
Please feel free to re-open this issue at your discretion.