- 
                Notifications
    You must be signed in to change notification settings 
- Fork 83
Resolve Credo issues and enforce it in CI #2258
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
base: main
Are you sure you want to change the base?
Resolve Credo issues and enforce it in CI #2258
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your recent PRs.
I like where this is going.
I think it might be going a little far in some cases. Credo is a good guide, as long as it does impact code clarity. I think there are a bunch of alias changes we can skip, and maybe instead ignore the credo warnings of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving these to disabled means we won't see any output for them.
I would like to see the output but not have them fail credo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitHub Actions workflow update:
# Before
- name: Run Credo
  run: mix credo
# After  
- name: Run Credo
  run: mix credo --min-priority lowIgnored Priority Checks - Excluded from CI
📝 Design.TagTODO
# TODO comments are acceptable in our codebase, but we want to see them locally
{Credo.Check.Design.TagTODO, [priority: :ignore]},📚 Readability.ModuleDoc
Missing @moduledoc tags
# ModuleDoc is not required in our codebase, but we want to see them locally  
{Credo.Check.Readability.ModuleDoc, [priority: :ignore]},Key Change: Using priority: :ignore ensures these checks don't block CI but remain visible for local development feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshk  I think the priority: :ignore can help as to resolve that issue together with mix credo --min-priority low
e846469    to
    78297ea      
    Compare
  
    | @joshk, first of all, thanks for the feedback. I tried to resolve all your comments at PR. I would say I'm ready for a second round of review. | 
5b9c493    to
    11b7d48      
    Compare
  
    | Just wanted to put Quokka out there as an option. It will automatically keep your codebase in line with your credo configuration so that you don't have to manually fix things 🎉 | 
| 
 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work on this, and sorry for the delay in reviewing the PR.
54ede78    to
    911fd2d      
    Compare
  
    | Sorry for the delay in merging in the Macros PR. Ping me when your ready for me to review this again. | 
cc4a3b4    to
    4a13fe5      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshk thanks for all you work, that's awesome to see the support from your side.
I did a rebase and you can review PR again. Thanks
9aeb142    to
    b3dba5c      
    Compare
  
    | @joshk I’ve updated the branch with the latest changes. Could you please take another look at the PR when you have a chance? Thanks! | 
b3dba5c    to
    63461e7      
    Compare
  
    | @@ -1,20 +1,20 @@ | |||
| defmodule NervesHubWeb.API.ErrorJSONTest do | |||
| @moduledoc false | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really needed for a test file? Do other tests no need this? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM! @joshk do you have any other thoughts before we merge?
🎯 Objective
Improve code quality and enforce coding standards by making Credo checks mandatory in CI, while resolving all existing code quality issues.
🔧 Changes
Enforced Credo in CI
--mute-exit-statusfrom.github/workflows/ci.ymlon--min-priority low(full comment here Resolve Credo issues and enforce it in CI #2258 (comment))#2258 (comment)
🐛 Credo Issues Fixed
Software Design Issues (21 files)
aliasstatements for nested modules likeX509.Certificate.Extension,Phoenix.Channel.Server,NervesHub.Telemetry.Customizationslib/nerves_hub/certificate.ex,lib/nerves_hub_web/plugs/configure_uploads.ex,lib/nerves_hub/extensions/health.exReadability Issues (6 files)
lib/nerves_hub/firmwares/update_tool/fwup.exdef spec()inlib/nerves_hub_web/api_spec.ex578088→578_088)Refactoring Opportunities (3 files)
friendly_blocked_until/1inlib/nerves_hub_web/components/device_update_status.exusing pattern matchingupdated_live_view/0andhooked_component/1into smaller, focused functionsEnum.map |> Enum.joinwithEnum.map_joinin testsConfiguration Updates
TODOcomments and@moduledoctags optional in.credo.exsTODOcomments are acceptable in our codebase for tracking technical debt, and@moduledocrequirements would be too disruptive for existing modules✅ Results