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 opts allow a custom category #1165

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rNoz
Copy link
Contributor

@rNoz rNoz commented Nov 28, 2024

What am I doing?

Introducing support for a custom category option in the format_issue function, so we can overwrite the category given by Credo.Check use spec.

Why?

This allows defining specific categories as part of the same Credo.Check, which it can be useful for uses cases where the "searching" algorithm is used for related checks, but with different categories and/or notification levels. For instance, a Credo.Check (*1) I have been implementing recently: it searches for soft-deprecated and forbidden functions (while having --warning-as-error enabled in our CI pipelines, so @deprecated cannot be used for soft ones):

  • Informational Check: Used for recommendations "avoid if possible, port it when you have time", categorized separately (e.g., :soft_deprecated, recommendation, advice, etc), and does not block CI pipelines (exit status 0). It can suggest alternative functions (when provided), and the owners of that region of code can update whenever they have time. Soft-deprecating functions take time to update all usages, and in most cases it requires cross-squad efforts, so, better to warn without failing and leave enough time for the developers to update their code (and even indirect usages, like external services that depend on the original functions, like grafana, datadog, new relic, etc).
  • Warning Check: Issues a warning and blocks the CI pipeline (non-zero exit status). The main function and category of the checker, "avoid always and right now".

By combining these checks under one implementation, we optimize performance and reduce code duplication, as 90% of the logic is shared. However, the checks remain distinct in their categories and purposes, balancing performance and maintainability against separation of concerns and clarity.

Original proposal and context in this PR that fixes the exit_status.

Let me know if further refinements are needed!

*1 Note: Let me know if you would be interested in that Credo.Check and having it as one of the default ones in credo. I will need more time to have it polished and as fast as possible, but I'll happy to do so this winter - early 2025 ;)

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.

1 participant