Repository navigation
Conversation
Add mika_credo_rules (GitHub source, hex 0.1.0 only ships 14 of the 99
checks) and enable every check that applies to a plain Elixir library.
Checks that target consumers of this library rather than the library
itself (NoRawEts, CacheRequiresSandboxOption, CacheOptsNoHardcodedUri,
NoDirectErlangRpc, NoReimplementedHelper) are listed as disabled.
Code changes driven by the new checks:
- RefreshAhead and HashRing spawn their background tasks from a
Task.Supervisor in the strategy's supervision tree.
- RingMonitor defers ETS seeding and node monitoring to handle_continue/2.
- Adapter rescues narrow to ArgumentError, the only exception ETS,
counters, persistent_term and ConCache raise; HashRing logs an RPC
exception before falling back.
- Table-owner processes use spawn_link instead of a bare {:ok, pid} match
on Task.start_link/1; DETS exits with the open_file reason.
- function_exported?/3 is guarded by Code.ensure_loaded?/1.
- Application env reads go through Cache.Config; Logger messages carry
the module prefix and inspect their values.
Tests replace Process.sleep with Cache.Wait.until polling, :sys.get_state
syncs and a gated refresh callback for the dedup test; the app-env
fixtures move to test_helper.exs.
Claude-Session: https://claude.ai/code/session_014FgvF2AKpSdWdLEEQQjhjZ
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #55 +/- ##
==========================================
+ Coverage 83.87% 87.09% +3.22%
==========================================
Files 24 24
Lines 707 713 +6
==========================================
+ Hits 593 621 +28
+ Misses 114 92 -22 ☔ View full report in Codecov by Harness. |
Codecov's patch check counted the retyped rescue clauses as new lines and reported 51% patch coverage. Exercise the reachable ones: ETS and Counter calls against a table that does not exist, ETS rehydration from a file with the wrong table name or garbage contents, DETS open_file failure, and RefreshAhead get/delete without a tracker table. Cache.PersistentTerm drops its rescue clauses instead: :persistent_term get/2, put/2 and erase/1 never raise, so they were dead code. Claude-Session: https://claude.ai/code/session_014FgvF2AKpSdWdLEEQQjhjZ
Dropping the PersistentTerm rescue clauses narrowed its return type, and
Elixir 1.20 then flagged the generated {:error, _} clause of
handle_adapter_result/3 as unreachable for that adapter, which fails the
CI compile under --warnings-as-errors. The clauses stay so every adapter
honours the same {:ok, _} | {:error, %ErrorMessage{}} contract.
The ConCache rescue clauses are exercised by deleting the cache's public
ETS table out from under it, which is the only way ConCache raises.
IsolationTest and ExtraCachesTest both sandbox TestCache, and Cache.SandboxRegistry.start/1 adopts an already-running sandbox Agent into the calling test's supervisor, so with async: true one test's teardown killed the other's cache mid-run. Declare them async: false and retire BlitzCredoChecks.NoAsyncFalse, which MikaCredoRules.AsyncTrueRequired supersedes: the concurrency choice must be explicit, and a deliberate async: false is allowed.
Task.start_link/1 keeps the owner's $callers and $ancestors metadata and is
the documented way to start a linked process here. The bare {:ok, pid} match
is fine: Task.start_link/1 cannot return an error tuple. The
NoBarePatternMatchOnFallible finding on these three sites will be handled in
the credo rule itself.
NoBarePatternMatchOnFallible now allows crash-preferable start functions,
so the {:ok, pid} = Task.start_link(...) matches in the ETS, DETS and
Counter adapters are no longer flagged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mika_credo_rulesand enables every check that applies to a plain Elixir library. The dep points at GitHub because hex0.1.0only ships 14 of the 99 checks; switch it back to hex once a new version is published.NoRawEts,CacheRequiresSandboxOption,CacheOptsNoHardcodedUri,NoDirectErlangRpc,NoReimplementedHelper.ErrorMessageRequiredexcludeslib/cache/ets.exbecauseopts_definition/1is a NimbleOptions custom validator whose contract is{:error, String.t()}.SingleModulePerFileexcludes the CI-only fixture filelib/cache_test_modules.ex.BlitzCredoChecks.StrictComparison,TagTODOandTagFIXMEare superseded by the equivalent mika checks and turned off to avoid double reports.Code changes
Cache.RefreshAheadandCache.HashRingrun their background tasks under aTask.Supervisorin the strategy's supervision tree instead ofTask.start/1.Cache.HashRing.RingMonitorseeds its ETS table and calls:net_kernel.monitor_nodes/2fromhandle_continue/2.rescueclauses narrow toArgumentError, the only exception:ets,:counters,:persistent_termandConCacheraise.Cache.HashRing.rpc_get/6logs a raised exception before returning:unavailable.spawn_link/1instead of{:ok, pid} = Task.start_link/1;Cache.DETSexits with the:dets.open_file/2reason instead of aMatchError.function_exported?/3calls are guarded byCode.ensure_loaded?/1; app env reads go throughCache.Config.fetch_env!/2;Cache.ETSLogger messages carry the module prefix andinspect/1their values.Tests
Process.sleepis gone:Cache.Wait.until/2(newtest/support/wait.ex) polls on observable state,:sys.get_state/1syncs GenServer mailboxes, and the refresh-ahead dedup test gates its refresh callback so the concurrent reads are provably in flight.CacheTestwaits for the adapter to answer a read before testing, which is what the removed 100ms sleep was covering forCache.Redis's async connect.test_helper.exs.async: true.Verification
mix credo --strict: no issues (81 checks).mix dialyzer: 0 errors.mix test: every non-Redis test green across 11 sequential local runs. Local Redis tests flake onmaintoo (Redix.ConnectionError: closedin 3 of 5 baseline runs), so CI is the arbiter there.https://claude.ai/code/session_014FgvF2AKpSdWdLEEQQjhjZ