rrrene / credo-proposals

Proposals for Credo, the Elixir code analysis tool with a focus on code consistency and teaching
MIT License
9 stars 0 forks source link

Feature: Add credo ignore file #85

Open bradleygolden opened 1 year ago

bradleygolden commented 1 year ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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. :+1:

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 commented 9 months ago

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 commented 7 months ago

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. :+1:

bradleygolden commented 6 months ago

Great thank you!