ci: enforce cyclomatic complexity ceiling with strict linting - #863
Closed
leandrocp wants to merge 1 commit into
Closed
ci: enforce cyclomatic complexity ceiling with strict linting#863leandrocp wants to merge 1 commit into
leandrocp wants to merge 1 commit into
Conversation
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
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.
Adds a cyclomatic complexity ceiling and turns the linter up to strict, so both
run on every push and pull request. Part of applying one complexity standard
across the projects in this family.
What this enforces
.credo.exspinsCredo.Check.Refactor.CyclomaticComplexityat max 9 —classic McCabe, the same metric and ceiling that oxlint's
eslint/complexityenforces on the JavaScript in the sibling repositories.
strict: truelives in the config rather than only in the CI invocation, so alocal
mix credoreports exactly what CI reports.Credo.Check.Readability.MaxLineLengthis set to Credo's strict default of120, or to
.formatter.exs'sline_lengthwhere that is larger, so theformatter and the linter cannot disagree about a line the formatter produced.
Credo.Check.Design.TagTODO/TagFIXMEare off. Those notes belong in theissue tracker; failing a build on one only encourages deleting the note.
assets/.oxlintrc.jsonturns on oxlint'scorrectness,pedantic,perfand
suspiciouscategories and pinseslint/complexityat the same max 9,classic McCabe, over the JavaScript in
assets/.Two checks are waived, each with the reason and the count in the config:
Credo.Check.Design.AliasUsage(109 sites — a naming question, not a complexityone, and a sweep would touch most of the tree at once) and
Credo.Check.Warning.StructFieldAmount(1 site,Beacon.Configat 42 fields,where every field is one documented option).
assets/.eslintrc.jsis replaced: nothing ran it, there was no lint script andno workflow.
oxlintgoes intodevDependencieswith annpm run lintscriptand a
mix assets.lintalias.CI
qualityjob inquality.ymlgains amix credo --strictstep.javascript.ymlruns oxlint overassets/, straight from the registrywith
npx oxlint@1.80.0.Code changes
Nineteen functions were over the cyclomatic ceiling and forty-nine were nested
deeper than Credo allows. Twenty-six of the latter were in
runtime_renderer.ex, the runtime AST interpreter.The largest ones:
Content.validate_if_value_matches_type/4(28) was a 20-armcondpairing atype name with its guard and its error message. The guards are now clauses of
value_matches_type?/2and the article istype_article/1; an unknown typestill raises, as it did.
Layouts.do_build_seo_meta_tags/3(24) built a list with eleventags = if non_empty?(x), do: [tag | tags], else: tagslines. It is now apipeline of
put_tag/4calls, in the same order, so adding a tag is one line.SEO.Metrics.compute/1(21) mixed four count queries, a per-page score andthe result map. The queries are named (
orphan_count/2and friends) and thescore is
page_score/1overscore_checks/1.RuntimeRenderer.eval_ast/2for|>(17) chose the module for a bare pipedcall inside a six-arm
cond; that is nowpipe_module/2, one clause perknown helper.
Css.ThemeParser.parse_value/2(19) was a five-armcondeach containing itsown nested
case; each arm is now its own function.Actions.Interpreter.evaluate_test/2(16) dispatched ten comparisonoperators in a
case; they are clauses ofcompare/3.RuntimeRenderer.do_render_component/3had the same twenty lines of "read theserialized component and render it" before and after the lazy load. That is
render_serialized_component/3, called from both.extract_component_assigns/1had two verbatim copies of the same nested slottransform. Both call
transform_component_pair/1andtransform_slot/1.eval_irfor component calls was a third copy ofbuild_cms_inner_block/3; it calls it now.Web.Live.PageLive.render/1nested acaseon the notification componentinside a
caseon the site setting inside anif; the four outcomes are fournamed functions.
Alongside those: ten
defp f do try do ... end endbodies became the implicitfunction-level
rescue, sixunless ... elsebecameif, fourlength(list) > 0became list patterns, twoEnum.count(x) > 0becameEnum.empty?/1, threewithexpressions with one clause and anelsebecamecase, twoEnum.map |> Enum.joinbecameEnum.map_join, alias groups weresorted in four files, and eight modules gained
@moduledoc false.Client.Filtersdefinesapply/3as its public entry point, and Credo readsthe heads of the clauses whose third argument is a list as calls to
Kernel.apply/3. That file carries acredo:disable-for-this-filewith thereason.
In JavaScript,
assets/js/beacon.jsmoved offgetElementById,getElementsByTagName(...)[0].appendChildand barehasOwnProperty.Verification
mix credo --strict: clean, 2861 modules and functions.mix test: 535/580 passed, 45 failed — a subset of the 46 that fail onmainbefore this branch. The one that no longer appears is flaky acrossruns on
maintoo. This checkout cannot reproduce a green suite; the failuresare unrelated to these changes.
mix compile --warnings-as-errors: the warning set is byte-identical tomain's. It is non-empty on both, on the Elixir this was written against.npx oxlint@1.80.0inassets/: clean.Files whose only difference would have been
mix formatoutput are left alone,so the diff is the change and not a reformat.