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

Credo.Check format_issue can use a custom exit_status option #1162

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

rNoz
Copy link
Contributor

@rNoz rNoz commented Nov 19, 2024

What am I doing?

Being able to use a custom exit_status option.

Why?

When I try to pass a custom :exit_status value to the format_issue opts, it is never applied. Am I missing something?

format_issue(
  issue_meta,
  message: message,
  trigger: mfa,
  exit_status: 0,
  line_no: line_no
)
# exit status is not 0

The tricky thing is that I am defining a Check that could generate:

  • 2 types of categories.
  • 2 different exit statuses (0 and 16 - warning).

Alternative approaches

How am I bypassing this for now? I am developing a Credo.Check and I do it by modifying the IssueMeta tuple, setting these values, which is a bit "weird", compared with just giving a custom option when formatting it.

As :exit_status is not established when using it (use options), it gets it from the function generated by the macro.

  @doc false
  def exit_status(params, check_mod) do
    params[:__exit_status__] || params[:exit_status] || check_mod.exit_status()
  end

Example:

exit_status = 0 # or the other coming from :warning
issue_meta = case issue_meta do
  {mod, source, params} -> {mod, source, Keyword.put(params, :exit_status, exit_status)
  issue_meta -> issue_meta
end
    
format_issue(issue_meta, ...)

@rrrene rrrene merged commit 47cad6b into rrrene:master Nov 20, 2024
18 of 19 checks passed
@rrrene
Copy link
Owner

rrrene commented Nov 20, 2024

@rNoz Thx for highlighting and fixing this. I found that :priority was also ignored: b9a76f1

This is remarkable, as this code hasn't been touched in 9 years. It was simply wrong the whole time 😳

@rNoz
Copy link
Contributor Author

rNoz commented Nov 21, 2024

You are welcome, @rrrene. I have another proposal that will facilitate modifying the category directly via the format_issue. This will be really useful when having a Credo.Check that can produce a variety of categories/issues. Can I send a proposal to include it as another option?

@rrrene
Copy link
Owner

rrrene commented Nov 22, 2024

@rNoz I think that could be a good idea, but I would want to ask the question: Are there other attributes that we should be able to set through format_issue/2?

Another question to ask is: Why is a single check belonging to two categories in your case? We had cases in Credo where a single check was producing two kinds of issues and even in that case, we eventually split the check in two to have a clear 1:1 relationship. (this is just me being curious, not trying to patronize 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

Successfully merging this pull request may close these issues.

2 participants