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

Feature: Add credo ignore file #85

Open
bradleygolden opened this issue Nov 5, 2022 · 6 comments
Open

Feature: Add credo ignore file #85

bradleygolden opened this issue Nov 5, 2022 · 6 comments

Comments

@bradleygolden
Copy link

bradleygolden commented Nov 5, 2022

What do you want Credo to do?

I want to ignore pre-existing failing checks without having to use mix credo diff because it's slow.

Mix credo on a branch without diffing:

mix credo  46.82s user 17.73s system 271% cpu 23.751 total

Mix credo on a branch with diffing:

mix credo diff --from-git-merge-base origin/master  160.56s user 33.53s system 204% cpu 1:35.07 total

This shows that diff checks are 4x slower than vanilla mix credo in my case! It's even worse when I run the check in github actions where the machine is much smaller than my local machine in terms of compute and memory.

Which existing behaviour would change?

  1. Have a .credo_ignore.exs or similar file like .dialyzer_ignore.exs or .sobelow_skips where ignored credo checks can live. I think this would require some kind of additional CLI functionality on credo's part to output failing checks in a format that can be used in the ignore file. Or maybe credo can create the ignore file automatically?
  2. Add ignore comments directly to the failing code. For example, if a line of code is failing, a credo ignore comment would be placed directly above the culprit line. I can see this implementation possibly getting messy.
  3. Manually ignore all checks via comment myself. This is not ideal and doesn't require any credo changes.

Out of these options I'd much prefer 1. the most. I think it would be the simplest to implement and an ignore file is great IMO because it documents all failing credo checks in a central location. It also keeps the code free of comments which I'm a fan of.

@bradleygolden bradleygolden changed the title Feature: Make ignoring many failing checks easy Feature: Make ignoring many existing failing checks easy and fast Nov 5, 2022
@bradleygolden bradleygolden changed the title Feature: Make ignoring many existing failing checks easy and fast Feature: Add credo ignore file Nov 7, 2022
@bradleygolden
Copy link
Author

I ended up figuring out how to do this via plugin. I might make the implementation open source after I test it internally for a bit. Here's the implementation for anyone out there that wants to ignore checks/extend the functionality I wrote:

# lib/credo_ignore_file_plug.ex
defmodule CredoIgnoreFilePlugin do
  @moduledoc false

  import Credo.Plugin

  def init(exec) do
    exec
    |> append_task(
      Credo.CLI.Command.Suggest.SuggestCommand,
      :filter_issues,
      CredoIgnoreFilePlugin.IgnoreIssues
    )
  end
end
# lib/credo_ignore_file_plugin/ignore_issues.ex
defmodule CredoIgnoreFilePlugin.IgnoreIssues do
  @moduledoc false

  use Credo.Execution.Task

  def call(exec, _opts) do
    ignore_issues = ignore_file_issues()

    kept_issues =
      exec
      |> Execution.get_issues()
      |> Enum.reject(&ignore_issue?(&1, ignore_issues))

    Execution.put_issues(exec, kept_issues)
  end

  defp ignore_file_issues do
    File.read!(".credoignore") |> Jason.decode!(keys: :atoms) |> Map.fetch!(:issues)
  end

  # this implementation isn't optimal but works well enough for me
  defp ignore_issue?(issue, ignore_issues) do
    Enum.any?(ignore_issues, fn ignore_issue ->
      issue.filename == ignore_issue.filename &&
        issue.line_no == ignore_issue.line_no &&
        issue.column == ignore_issue.column &&
        check_name(issue.check) == ignore_issue.check
    end)
  end

  defp check_name(check) do
    check
    |> to_string()
    |> String.replace(~r/^(Elixir\.)/, "")
  end
end

To use this code you can generate the ignore file with mix credo list --format json > .credoignore followed by mix credo.

You can follow credo's plugin guide for instructions on how to use this code. I found that I had to create a library rather than use the modules by themselves like you can with custom credo checks.

@bradleygolden
Copy link
Author

I went head and open sourced my code. I'd love to hear your feedback if you have any!

https://hex.pm/packages/credo_ignore_file_plugin

@rrrene
Copy link
Owner

rrrene commented Dec 27, 2023

So this does allow you to record the state of a project in terms of Credo issues and then ignore these issue, basically starting with a clean slate?

Sounds interesting (and reminds me of rubocop_todo.yml from my Ruby days. 👍

One thing I noticed: You probably get into trouble when comparing line numbers and columns of issues, because just adding a comment somewhere in a file will result in that issue being no longer ignored.

@bradleygolden
Copy link
Author

So this does allow you to record the state of a project in terms of Credo issues and then ignore these issue, basically starting with a clean slate?

Yes.

One thing I noticed: You probably get into trouble when comparing line numbers and columns of issues, because just adding a comment somewhere in a file will result in that issue being no longer ignored.

Yes, this is an issue. It might be nice to make line numbers and/or column numbers optional in this case.

@rrrene
Copy link
Owner

rrrene commented Mar 5, 2024

I found that I had to create a library rather than use the modules by themselves like you can with custom credo checks.

Going throuogh the proposals, I found this remark and updated the documentation on how to do it without creating a Hex package. 👍

@bradleygolden
Copy link
Author

Great thank you!

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

No branches or pull requests

2 participants