feat(build): enhance build system with linting, coverage, and CI workflows #108
Conversation
WalkthroughThis update introduces automated style, grammar, and spelling checks to the build process, adds test coverage tooling and reporting, and enhances documentation to explain these workflows. New scripts and configuration files support markdown linting, spell checking, grammar validation, and Go coverage aggregation. The Makefile and documentation are updated to integrate and describe these features. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Make as Makefile
participant Lint as Lint/Check Tools
participant Test as Go Test & Coverage Scripts
participant CI as GitHub Actions
Dev->>Make: make tidy
Make->>Lint: Run whitespace, markdown, grammar, spelling checks
Lint-->>Make: Return results
Dev->>Make: make coverage
Make->>Test: Run make_coverage.sh (per-module)
Test->>Test: Aggregate coverage profiles
Test->>Make: Output coverage.out and coverage.html
CI->>Make: Trigger on push/PR
Make->>Test: Run tests and coverage
Test->>CI: Upload coverage to Codecov
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Here's the code health analysis summary for commits Analysis Summary
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (12)
internal/build/cspell.json (2)
5-50: Deduplicate mixed-case entries to cut down noise in reports
PNPX/pnpxandStrs/strsare listed twice, only differing by capitalisation.
Unless you intentionally want both cases whitelisted, keep a single, lower-case variant – cspell treats the list case-sensitively, so the second entry is redundant.- "PNPX", - "pnpm", - "pnpx", + "pnpm", + "pnpx", … - "Strs", - "strs", + "strs",
51-58: Consider ignoring generated coverage artefacts here as well
coverage.out/coverage.htmlare now produced bymake coverage; adding them toignorePathsprevents cspell from scanning large blobs of data when the developer forgets to clean the working tree."vendor" + ,"coverage.out" + ,"coverage.html"internal/build/languagetool.cfg (1)
5-15: Rule list is hard to maintain – format one-per-lineA comma-separated list makes future diffs painful. Place each rule on its own line; LanguageTool accepts multi-line TOML/INI values.
-disabledRules = COMMA_PARENTHESIS_WHITESPACE,ARROWS,EN_QUOTES,MORFOLOGIK_RULE_EN_GB,UPPERCASE_SENTENCE_START +disabledRules = \ + COMMA_PARENTHESIS_WHITESPACE, \ + ARROWS, \ + EN_QUOTES, \ + MORFOLOGIK_RULE_EN_GB, \ + UPPERCASE_SENTENCE_STARTinternal/build/markdownlint.json (1)
15-26: Add commonly-recommended rules to catch list / heading quirksRules such as
MD007(indentation of nested lists) andMD046(code-block style) help keep docs consistent and are inexpensive to adopt."MD041": true, - "MD047": true + "MD046": true, + "MD047": true, + "MD007": { "indent": 2 } }.deepsource.toml (1)
9-11: Nit: quote the import root to avoid accidental interpretationAlthough TOML allows bare strings, wrapping the module path in quotes avoids issues if a future ref adds dots or dashes.
-import_root = "github.com/darvaza-proxy/core" +import_root = "github.com/darvaza-proxy/core".github/workflows/codecov.yml (1)
20-24: Race detector + atomic cover mode doubles instrumentationRunning
-racetogether with-covermode=atomicincreases compile times and
memory; if you only need coverage for Codecov consider dropping-racehere
and keep it in a separate fast-fail job.internal/build/fix_whitespace.sh (2)
32-33: Sub-shell assigns + multiplestatvariants – quote and split safelyThe combined command substitution masks errors and fails Shellcheck. Simplify
and quote to avoid word-splitting.- size=$(stat -c%s "$file" 2>/dev/null || stat -f%z "$file" 2>/dev/null) + if size=$(stat -c%s -- "$file" 2>/dev/null); then :; \ + elif size=$(stat -f%z -- "$file" 2>/dev/null); then :; \ + else size=0; fi
40-42:xxdis not guaranteed presentMinimal containers often lack
xxd. Useodor fallback toprintf.- if [ "$(tail -c 1 "$file" | xxd -p)" != "0a" ]; then + last_byte=$(tail -c 1 "$file" | od -An -tx1 | tr -d ' \n') + if [ "$last_byte" != "0a" ]; thenAGENT.md (1)
162-168: Minor style inconsistencyBullets after line 167 mix sentence-ending periods with none. For the strict
Markdownlint rule added, end every list item with a period.internal/build/make_coverage.sh (1)
23-25: Unused loop fields trigger Shellcheck warnings
dir,mod, anddepsare parsed but never referenced, producing SC2034. Either prefix with an underscore or omit them.-while IFS=: read -r name dir mod deps; do +while IFS=: read -r name _dir _mod _deps; doMakefile (2)
136-138: Coverage artefacts are written to repo root
make_coverage.shalways emitscoverage.out(and optionallycoverage.html) in the working directory, but we invoke it with$(TMPDIR)/coverageas the output folder.
The mixed locations complicate cleanup and the.gitignorealready ignorescoverage.outat the root level, which now becomes stale for local runs.Pass an explicit output file path or push
coverage.outinto$(TMPDIR)/coverageto keep artefacts together.
80-85: Unnecessary subshell for file lists
$(shell find …)is executed every make invocation and slows the graph even for unrelated targets. Consider computing the lists lazily or caching them in.tmp, similar to the index generation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.deepsource.toml(1 hunks).github/workflows/codecov.yml(1 hunks).gitignore(1 hunks)AGENT.md(5 hunks)Makefile(4 hunks)README.md(3 hunks)internal/build/cspell.json(1 hunks)internal/build/fix_whitespace.sh(1 hunks)internal/build/gen_mk.sh(1 hunks)internal/build/languagetool.cfg(1 hunks)internal/build/make_coverage.sh(1 hunks)internal/build/markdownlint.json(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
README.md (2)
Learnt from: amery
PR: darvaza-proxy/core#105
File: sync.go:11-12
Timestamp: 2025-04-17T20:14:08.093Z
Learning: In the darvaza.org/core module, the SpinLock type is being deprecated in favor of darvaza.org/x/sync/spinlock. The implementation must remain in place (not an alias) to avoid circular dependencies, as the new package likely imports from core.
Learnt from: amery
PR: darvaza-proxy/core#82
File: errors.go:22-23
Timestamp: 2024-10-11T13:59:15.993Z
Learning: `ErrNilReceiver`, defined in `errors.go`, is intended for use by other packages even if it's not used within the current codebase.
internal/build/gen_mk.sh (2)
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
internal/build/fix_whitespace.sh (1)
Learnt from: karasz
PR: darvaza-proxy/core#74
File: internal/build/get_version.sh:21-30
Timestamp: 2024-10-01T08:39:29.235Z
Learning: In `get_version.sh`, focus on addressing necessary changes, such as Shellcheck warnings, and avoid suggesting optional readability improvements or stylistic changes unless they address critical issues.
AGENT.md (3)
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: karasz
PR: darvaza-proxy/core#74
File: internal/build/get_version.sh:21-30
Timestamp: 2024-10-01T08:39:29.235Z
Learning: In `get_version.sh`, focus on addressing necessary changes, such as Shellcheck warnings, and avoid suggesting optional readability improvements or stylistic changes unless they address critical issues.
internal/build/make_coverage.sh (3)
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: karasz
PR: darvaza-proxy/core#74
File: internal/build/get_version.sh:21-30
Timestamp: 2024-10-01T08:39:29.235Z
Learning: In `get_version.sh`, focus on addressing necessary changes, such as Shellcheck warnings, and avoid suggesting optional readability improvements or stylistic changes unless they address critical issues.
🧬 Code Graph Analysis (1)
internal/build/make_coverage.sh (1)
internal/build/gen_index.sh (1)
mod(10-13)
🪛 Shellcheck (0.10.0)
internal/build/fix_whitespace.sh
[warning] 18-18: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 32-32: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 32-32: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 32-32: Quote this to prevent word splitting.
(SC2046)
[warning] 35-35: This redirection doesn't have a command. Move to its command (or use 'true' as no-op).
(SC2188)
internal/build/make_coverage.sh
[warning] 23-23: dir appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 23-23: mod appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 23-23: deps appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (9)
internal/build/markdownlint.json (1)
4-14: 80-column hard limit may be too aggressive for code blocks
"strict": truecombined with an 80-char limit will fail on Go import lines and long URLs. If the intention is to enforce 80 for prose only, disablecode_blocksor raise thecode_block_line_length..gitignore (1)
1-5:*.testmay hide non-Go artefacts – double-checkThe pattern will ignore every file ending in
.test, not just Go test binaries.
If that breadth is unintended, prefer*.test.exe(Windows) /*.testproduced bygo test -cinside specific directories.README.md (3)
181-182: Formatting-only change – looks goodLine break inserted around the
Panic*helpers improves readability without altering semantics.
220-221: Consistent formatting retainedBullet wraps cleanly at 80 chars; nothing further to flag.
272-273: Minor doc tweak OKBritish-English spelling of “favour” retained; link renders fine.
internal/build/gen_mk.sh (1)
135-136: GOTEST_FLAGS injection is fine but quote to avoid word-splittingIf
GOTEST_FLAGSever contains multiple values with inner spaces (e.g.
"-coverpkg=./... -run TestFoo"), Make will expand them correctly, but the
shell re-parses the variable causing additional word-splitting. Wrap the
variable in double quotes to keep each flag group intact.- call="$(GO) $cmd $(GOTEST_FLAGS) ./..." + call="$(GO) $cmd $(GOTEST_FLAGS) ./..." + # ^^^^^^^^^^^^^^^ +# alternatively, honour internal spaces +# call="$(GO) $cmd $(strip $(GOTEST_FLAGS)) ./..."⛔ Skipped due to learnings
Learnt from: amery PR: darvaza-proxy/core#79 File: internal/build/gen_mk.sh:161-169 Timestamp: 2024-10-08T15:33:35.792Z Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.Learnt from: amery PR: darvaza-proxy/core#79 File: internal/build/gen_mk.sh:161-169 Timestamp: 2024-10-01T19:43:25.764Z Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly..github/workflows/codecov.yml (1)
15-19: Targeting unreleased Go 1.23 may break CI
actions/setup-go@v5will fall back to the latest stable version it can find,
but pinning to1.23makes the workflow fragile until 1.23 is actually on the
download servers. Prefer the latest released minor or a version matrix.- go-version: '1.23' + # Use latest stable or test against several versions + go-version: '1.22.x' +# matrix: +# go-version: [ '1.21.x', '1.22.x' ]internal/build/fix_whitespace.sh (1)
16-24:localis not POSIX; script breaks on/bin/sh(dash, busybox, …)
#!/bin/shpromises POSIX;localis a bash/ksh extension. On systems where
shisdashthe function exits with “local: not found”.-fix_file() { - local file="$1" +fix_file() { + file=$1⛔ Skipped due to learnings
Learnt from: amery PR: darvaza-proxy/core#75 File: internal/build/get_version.sh:0-0 Timestamp: 2024-10-08T15:33:35.792Z Learning: In this project, the use of the `local` keyword and `-a` in the `test` command is acceptable in shell scripts, and POSIX compliance is not a concern.Learnt from: amery PR: darvaza-proxy/core#75 File: internal/build/get_version.sh:0-0 Timestamp: 2024-10-01T15:48:48.786Z Learning: In this project, the use of the `local` keyword and `-a` in the `test` command is acceptable in shell scripts, and POSIX compliance is not a concern.Learnt from: karasz PR: darvaza-proxy/core#74 File: internal/build/get_version.sh:21-30 Timestamp: 2024-10-01T08:39:29.235Z Learning: In `get_version.sh`, focus on addressing necessary changes, such as Shellcheck warnings, and avoid suggesting optional readability improvements or stylistic changes unless they address critical issues.internal/build/make_coverage.sh (1)
42-46: Command-line length may explode on many modules
gocovmerge $coverage_filesexpands the entire list into one argument vector; with hundreds of modules you risk “Argument list too long”. Usexargs(supported bygocovmerge) to stream the list.- gocovmerge $coverage_files > coverage.out + printf '%s\n' $coverage_files | xargs gocovmerge > coverage.out
amery
force-pushed
the
pr-amery-buildsys
branch
2 times, most recently
from
305031b to
bbf9ddc
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
internal/build/fix_whitespace.sh (1)
28-30: Repeat:sed -iwithout backup suffix is not portable
BSDsed(macOS) rejects-iwith no argument. Earlier review already pointed this out.internal/build/make_coverage.sh (1)
64-64:set -o pipefailnot portable – see first comment
Secondary reminder that this line explodes underdash.
🧹 Nitpick comments (3)
internal/build/fix_whitespace.sh (1)
39-42:tail -c 1is non-standard; rely on POSIXddinstead- if [ -n "$(tail -c 1 "$file" 2>/dev/null)" ]; then + if dd if="$file" bs=1 skip=$((size-1)) count=1 2>/dev/null | grep -q '[^ ]'; thenThis keeps the script strictly POSIX.
internal/build/make_coverage.sh (1)
24-27: Word-splitting risk when reading module list
for name in $(cut …)splits on IFS; a module path containing spaces (rare but legal) would explode. Safer:while IFS=: read -r name _; do [ -z "$name" ] && continue … done <"$INDEX"Makefile (1)
107-112: Whitespace fixer will also touch generated artefacts
$(FIX_WHITESPACE_ARGS)defaults to.so it traverses$(TMPDIR)and.coverage, rewriting large auto-generated files and wasting CI time. Exclude build/output dirs:-FIX_WHITESPACE_ARGS ?= . \! \( $(FIX_WHITESPACE_EXCLUDE) \) +FIX_WHITESPACE_ARGS ?= . \( -path $(TMPDIR) -o -path .coverage \) -prune -o \! \( $(FIX_WHITESPACE_EXCLUDE) \)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/codecov.yml(1 hunks).gitignore(1 hunks)AGENT.md(5 hunks)Makefile(4 hunks)internal/build/cspell.json(1 hunks)internal/build/fix_whitespace.sh(1 hunks)internal/build/languagetool.cfg(1 hunks)internal/build/make_coverage.sh(1 hunks)internal/build/markdownlint.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- .gitignore
- internal/build/languagetool.cfg
- .github/workflows/codecov.yml
- internal/build/cspell.json
- internal/build/markdownlint.json
- AGENT.md
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Makefile (2)
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
internal/build/fix_whitespace.sh (3)
Learnt from: karasz
PR: darvaza-proxy/core#74
File: internal/build/get_version.sh:21-30
Timestamp: 2024-10-01T08:39:29.235Z
Learning: In `get_version.sh`, focus on addressing necessary changes, such as Shellcheck warnings, and avoid suggesting optional readability improvements or stylistic changes unless they address critical issues.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
internal/build/make_coverage.sh (3)
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: karasz
PR: darvaza-proxy/core#74
File: internal/build/get_version.sh:21-30
Timestamp: 2024-10-01T08:39:29.235Z
Learning: In `get_version.sh`, focus on addressing necessary changes, such as Shellcheck warnings, and avoid suggesting optional readability improvements or stylistic changes unless they address critical issues.
🪛 GitHub Actions: Test and Coverage
Makefile
[error] 137-137: Makefile target 'coverage' failed with exit code 2 due to shell script error.
internal/build/make_coverage.sh
[error] 64-64: Shell script error: 'set: Illegal option -o pipefail' causing coverage step failure.
🪛 Shellcheck (0.10.0)
internal/build/fix_whitespace.sh
[warning] 18-18: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 32-32: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 32-32: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 32-32: Quote this to prevent word splitting.
(SC2046)
[warning] 33-33: Prefer [ p ] || [ q ] as [ p -o q ] is not well defined.
(SC2166)
[warning] 35-35: This redirection doesn't have a command. Move to its command (or use 'true' as no-op).
(SC2188)
internal/build/make_coverage.sh
[warning] 64-64: In POSIX sh, set option pipefail is undefined.
(SC3040)
🔇 Additional comments (2)
internal/build/fix_whitespace.sh (1)
16-22:localis non-POSIX – either switch to bash or drop it
#!/bin/shpromises a POSIX shell, yetlocalis abash/kshextension. On Debian/Ubuntu/bin/shisdash, which will abort here.-fix_file() { - local file="$1" +fix_file() { +file=$1 # POSIX-portable; no 'local'⛔ Skipped due to learnings
Learnt from: amery PR: darvaza-proxy/core#75 File: internal/build/get_version.sh:0-0 Timestamp: 2024-10-08T15:33:35.792Z Learning: In this project, the use of the `local` keyword and `-a` in the `test` command is acceptable in shell scripts, and POSIX compliance is not a concern.Learnt from: amery PR: darvaza-proxy/core#75 File: internal/build/get_version.sh:0-0 Timestamp: 2024-10-01T15:48:48.786Z Learning: In this project, the use of the `local` keyword and `-a` in the `test` command is acceptable in shell scripts, and POSIX compliance is not a concern.Learnt from: karasz PR: darvaza-proxy/core#74 File: internal/build/get_version.sh:21-30 Timestamp: 2024-10-01T08:39:29.235Z Learning: In `get_version.sh`, focus on addressing necessary changes, such as Shellcheck warnings, and avoid suggesting optional readability improvements or stylistic changes unless they address critical issues.Makefile (1)
136-138: Coverage target fails because underlying script exits non-zero
Oncemake_coverage.shis fixed (pipefail), CI should go green. No Makefile changes required.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/build/fix_whitespace.sh (1)
26-28:sed -iis still non-portable on BSD/macOS
Previous reviews already flagged this. Please switch to a temp-file strategy or add the zero-length backup suffix to keep the script cross-platform.- sed -i 's/[[:space:]]*$//' "$file" + # Portable in-place edit (works with GNU and BSD sed) + sed -e 's/[[:space:]]*$//' "$file" >"$file.tmp" && mv "$file.tmp" "$file"
🧹 Nitpick comments (3)
internal/build/fix_whitespace.sh (2)
29-35: ShellCheck SC2188: redirect without command
Pure redirection is legal, but a no-op command (:) suppresses the warning and is self-documenting.- > "$file" + : > "$file"
14-15: Addpipefailfor safer pipelines
set -euo pipefailprevents silent failures inside pipelines, especially important because the script pipesfind | while ….-set -eu +set -euo pipefailMakefile (1)
112-116:echo … | tr ' ' '\n'loses files containing spaces
Use NUL-delimited lists to be robust.- $Q echo "$(GO_FILES)" | tr ' ' '\n' | xargs -r $(GOFMT) $(GOFMT_FLAGS) + $Q printf '%s\0' $(GO_FILES) | xargs -r -0 $(GOFMT) $(GOFMT_FLAGS) … - $Q echo "$(MARKDOWN_FILES)" | tr ' ' '\n' | xargs -r $(MARKDOWNLINT) $(MARKDOWNLINT_FLAGS) + $Q printf '%s\0' $(MARKDOWN_FILES) | xargs -r -0 $(MARKDOWNLINT) $(MARKDOWNLINT_FLAGS)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/codecov.yml(1 hunks).gitignore(1 hunks)AGENT.md(5 hunks)Makefile(4 hunks)internal/build/cspell.json(1 hunks)internal/build/fix_whitespace.sh(1 hunks)internal/build/languagetool.cfg(1 hunks)internal/build/make_coverage.sh(1 hunks)internal/build/markdownlint.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- .gitignore
- .github/workflows/codecov.yml
- internal/build/cspell.json
- internal/build/markdownlint.json
- internal/build/languagetool.cfg
- AGENT.md
- internal/build/make_coverage.sh
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Makefile (1)
Learnt from: karasz
PR: darvaza-proxy/core#74
File: internal/build/get_version.sh:21-30
Timestamp: 2024-10-01T08:39:29.235Z
Learning: In `get_version.sh`, focus on addressing necessary changes, such as Shellcheck warnings, and avoid suggesting optional readability improvements or stylistic changes unless they address critical issues.
internal/build/fix_whitespace.sh (5)
Learnt from: karasz
PR: darvaza-proxy/core#74
File: internal/build/get_version.sh:21-30
Timestamp: 2024-10-01T08:39:29.235Z
Learning: In `get_version.sh`, focus on addressing necessary changes, such as Shellcheck warnings, and avoid suggesting optional readability improvements or stylistic changes unless they address critical issues.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#75
File: internal/build/get_version.sh:0-0
Timestamp: 2024-10-08T15:33:35.792Z
Learning: In this project, the use of the `local` keyword and `-a` in the `test` command is acceptable in shell scripts, and POSIX compliance is not a concern.
Learnt from: amery
PR: darvaza-proxy/core#75
File: internal/build/get_version.sh:0-0
Timestamp: 2024-10-01T15:48:48.786Z
Learning: In this project, the use of the `local` keyword and `-a` in the `test` command is acceptable in shell scripts, and POSIX compliance is not a concern.
🪛 Shellcheck (0.10.0)
internal/build/fix_whitespace.sh
[warning] 18-18: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 33-33: This redirection doesn't have a command. Move to its command (or use 'true' as no-op).
(SC2188)
🔇 Additional comments (1)
Makefile (1)
81-86:findoutputs captured at parse-time may go stale
BecauseMARKDOWN_FILESandGO_FILESare expanded whenmakeparses the file, newly-created files during the same run (e.g. by code-generation) will be missed. Consider computing the lists inside the recipe instead of with$(shell …)or add dependencies forcing a second pass.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.deepsource.toml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
.deepsource.toml (1)
Learnt from: karasz
PR: darvaza-proxy/core#74
File: internal/build/get_version.sh:21-30
Timestamp: 2024-10-01T08:39:29.235Z
Learning: In `get_version.sh`, focus on addressing necessary changes, such as Shellcheck warnings, and avoid suggesting optional readability improvements or stylistic changes unless they address critical issues.
🔇 Additional comments (2)
.deepsource.toml (2)
3-8:exclude_patternsentry may miss the most common Go test filesThe glob
**/*.testexcludes files literally ending with.test, but Go unit-test sources are named*_test.go.
If the intent is to keep Go tests out of static analysis you probably need an additional pattern, e.g.exclude_patterns = [ "**/*.test", + "**/*_test.go", ".tmp/**", ]Otherwise Go test files will still be inspected by DeepSource.
18-21: Dialect tag is useful but redundant for.shextensionsShell files already default to POSIX-sh dialect when the extension is
.sh.
Keeping the explicitdialect = "sh"is harmless, but if you maintain mixed dialects (bash, dash, busybox) consider directory-specific overrides rather than a single global setting.
amery
force-pushed
the
pr-amery-buildsys
branch
2 times, most recently
from
b5e15f3 to
65c0def
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
AGENT.md (3)
17-20: Conflicting minimum-Go version requirements
The prerequisites section lists “Go 1.22 or later” while the “Important Notes” section says “Go 1.23 is the minimum required version.” Please settle on a single version to avoid confusing contributors (and CI images).- - Go 1.22 or later installed (check with `go version`). + - Go 1.23 or later installed (check with `go version`). … - - Go 1.23 is the minimum required version. + - Go 1.23 is the minimum required version. <!-- keep only one statement -->Also applies to: 152-154
31-42: MentionGOTEST_FLAGSexplicitly
The examples showmake test GOTEST_FLAGS="-cover"/-v, which is great, but AGENT.md never explains whatGOTEST_FLAGSis or where it is honoured. Add a short one-liner so users know this is a passthrough togo test.
195-200: Bullet-list formatting is inconsistent with the documented standards
Here the Markdown bullets end without periods, whereas earlier you state that list items must end with a full stop. Either remove that rule or add the missing punctuation for self-consistency.Makefile (3)
32-39: Version detection for markdownlint may fail on “v0.” output**
grep -q '^[0-9]'matches “0.35.0” but not the more common “v0.35.0”. Consider stripping a leading “v” or loosening the regex to avoid false negatives that silently disable linting.-ifeq ($(shell $(PNPX) markdownlint-cli --version 2>&1 | grep -q '^[0-9]' && echo yes),yes) +ifeq ($(shell $(PNPX) markdownlint-cli --version 2>&1 | sed 's/^v//' | grep -q '^[0-9]' && echo yes),yes)
81-86:findexpression will not skip nestedvendor/paths
-name vendor -pruneonly matches a directory literally called “vendor” at the current depth; a nested path likeinternal/vendorslips through. Using-path '*/vendor'is safer.-MARKDOWN_FILES = $(shell find . \( -name vendor -o -name .git \) -prune -o -name '*.md' -print) -GO_FILES = $(shell find . \( -name vendor -o -name .git \) -prune -o -name '*.go' -print) +MARKDOWN_FILES = $(shell find . \( -path '*/vendor' -o -path '*/.git' \) -prune -o -name '*.md' -print) +GO_FILES = $(shell find . \( -path '*/vendor' -o -path '*/.git' \) -prune -o -name '*.go' -print)
112-116:fmttarget will mis-format ifGO_FILESis empty
echo "$(GO_FILES)" | tr ' ' '\n'emits a blank line, causingxargsto invokegofmtwith an empty argument list (error = 1). Guard with a test or useprintf '%s\0'+xargs -0.- $Q echo "$(GO_FILES)" | tr ' ' '\n' | xargs -r $(GOFMT) $(GOFMT_FLAGS) + $Q printf '%s\0' $(GO_FILES) | xargs -0 -r $(GOFMT) $(GOFMT_FLAGS)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.deepsource.toml(1 hunks).github/workflows/codecov.yml(1 hunks).gitignore(1 hunks)AGENT.md(5 hunks)Makefile(4 hunks)internal/build/cspell.json(1 hunks)internal/build/languagetool.cfg(1 hunks)internal/build/make_coverage.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- .gitignore
- .deepsource.toml
- internal/build/languagetool.cfg
- .github/workflows/codecov.yml
- internal/build/cspell.json
- internal/build/make_coverage.sh
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
AGENT.md (3)
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: karasz
PR: darvaza-proxy/core#74
File: internal/build/get_version.sh:21-30
Timestamp: 2024-10-01T08:39:29.235Z
Learning: In `get_version.sh`, focus on addressing necessary changes, such as Shellcheck warnings, and avoid suggesting optional readability improvements or stylistic changes unless they address critical issues.
Makefile (3)
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: karasz
PR: darvaza-proxy/core#74
File: internal/build/get_version.sh:21-30
Timestamp: 2024-10-01T08:39:29.235Z
Learning: In `get_version.sh`, focus on addressing necessary changes, such as Shellcheck warnings, and avoid suggesting optional readability improvements or stylistic changes unless they address critical issues.
🪛 LanguageTool
AGENT.md
[style] ~246-~246: Consider using a different verb for a more formal wording.
Context: ...markdownlint - If make tidy fails, fix the issues and run it again until it pa...
(FIX_RESOLVE)
[uncategorized] ~263-~263: Possible missing comma found.
Context: .... DeepSource shell issues: - Use ShellCheck disable comments for specific lines. ...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~263-~263: Possible missing preposition found.
Context: ...rce shell issues**: - Use ShellCheck disable comments for specific lines. - Updat...
(AI_HYDRA_LEO_MISSING_TO)
🔇 Additional comments (1)
Makefile (1)
10-10:GOTEST_FLAGSdeclared but never used in this Makefile
The variable is defined yet nothing in this file references it; usage presumably lives ingen.mk. If that’s intentional, add a comment here; otherwise wire it into the top-leveltest/coveragetargets to avoid a dead configuration knob.
…eyword - Set shell dialect to 'sh' for POSIX compliance - Add exclusion for SH-1091 (local keyword in POSIX sh) - Exclude .tmp/** and test files from analysis This reduces false positives while maintaining code quality checks for the shell scripts that use common but non-POSIX extensions like the 'local' keyword. Signed-off-by: Alejandro Mery <amery@apptly.co>
Update AGENT.md and README.md to comply with 80-character line length limit enforced by the improved markdownlint configuration. Signed-off-by: Alejandro Mery <amery@apptly.co>
Signed-off-by: Alejandro Mery <amery@apptly.co>
Add GOTEST_FLAGS variable to enable coverage testing, verbose output, and test binary building. Update gen_mk.sh to pass flags to go test command and document usage in AGENT.md. Also add *.test to .gitignore to exclude test binaries from version control. Signed-off-by: Alejandro Mery <amery@apptly.co>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/build/fix_whitespace.sh (1)
27-27: Non-portablesed -iusage remains
GNUsed -iwithout a backup suffix breaks on BSD/macOS. The earlier review already highlighted this; the command is still unchanged.- sed -i 's/[[:space:]]*$//' "$file" + # Portable in-place edit + sed -e 's/[[:space:]]*$//' "$file" >"$file.tmp" && mv "$file.tmp" "$file"
🧹 Nitpick comments (4)
internal/build/fix_whitespace.sh (1)
30-35: ShellCheck SC2188 – redirect with no command
> "$file"works, but triggers SC2188. A tiny no-op keeps linters quiet and is clearer.- > "$file" + : >"$file" # truncate fileMakefile (2)
30-38: Tool-detection shell calls fail hard whenpnpxis absent
Ifpnpxis missing,$(shell $(PNPX)… )prints “pnpx: command not found” to stderr during the parse phase, polluting CI logs. Wrap the test to silence the error:-ifeq ($(shell $(PNPX) markdownlint-cli --version 2>&1 | grep -q '^[0-9]' && echo yes),yes) +ifeq ($(shell command -v $(PNPX) >/dev/null 2>&1 && $(PNPX) markdownlint-cli --version 2>/dev/null | grep -q '^[0-9]' && echo yes),yes)
112-116:xargswithout-0may mis-handle unusual filenames
Follow-up to the previous point: switch to null-terminated streams.- $Q echo "$(GO_FILES)" | tr ' ' '\n' | xargs -r $(GOFMT) $(GOFMT_FLAGS) + $Q printf '%s\0' $(GO_FILES) | xargs -0 -r $(GOFMT) $(GOFMT_FLAGS)Same for the
markdownlintinvocation.AGENT.md (1)
304-307: Minor style: imperative mood for checklist
Use imperative verbs consistently (e.g., “Runmake tidy”, “Fix all issues”) to match other checklist items.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.deepsource.toml(1 hunks).github/workflows/codecov.yml(1 hunks).gitignore(1 hunks)AGENT.md(5 hunks)Makefile(4 hunks)README.md(3 hunks)internal/build/cspell.json(1 hunks)internal/build/fix_whitespace.sh(1 hunks)internal/build/gen_mk.sh(1 hunks)internal/build/languagetool.cfg(1 hunks)internal/build/make_coverage.sh(1 hunks)internal/build/markdownlint.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (8)
- .gitignore
- .deepsource.toml
- internal/build/cspell.json
- internal/build/languagetool.cfg
- .github/workflows/codecov.yml
- internal/build/gen_mk.sh
- internal/build/markdownlint.json
- internal/build/make_coverage.sh
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
AGENT.md (3)
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: karasz
PR: darvaza-proxy/core#74
File: internal/build/get_version.sh:21-30
Timestamp: 2024-10-01T08:39:29.235Z
Learning: In `get_version.sh`, focus on addressing necessary changes, such as Shellcheck warnings, and avoid suggesting optional readability improvements or stylistic changes unless they address critical issues.
Makefile (3)
Learnt from: karasz
PR: darvaza-proxy/core#74
File: internal/build/get_version.sh:21-30
Timestamp: 2024-10-01T08:39:29.235Z
Learning: In `get_version.sh`, focus on addressing necessary changes, such as Shellcheck warnings, and avoid suggesting optional readability improvements or stylistic changes unless they address critical issues.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
internal/build/fix_whitespace.sh (5)
Learnt from: karasz
PR: darvaza-proxy/core#74
File: internal/build/get_version.sh:21-30
Timestamp: 2024-10-01T08:39:29.235Z
Learning: In `get_version.sh`, focus on addressing necessary changes, such as Shellcheck warnings, and avoid suggesting optional readability improvements or stylistic changes unless they address critical issues.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#75
File: internal/build/get_version.sh:0-0
Timestamp: 2024-10-08T15:33:35.792Z
Learning: In this project, the use of the `local` keyword and `-a` in the `test` command is acceptable in shell scripts, and POSIX compliance is not a concern.
Learnt from: amery
PR: darvaza-proxy/core#75
File: internal/build/get_version.sh:0-0
Timestamp: 2024-10-01T15:48:48.786Z
Learning: In this project, the use of the `local` keyword and `-a` in the `test` command is acceptable in shell scripts, and POSIX compliance is not a concern.
🪛 LanguageTool
AGENT.md
[style] ~306-~306: Consider using a different verb for a more formal wording.
Context: ...markdownlint - If make tidy fails, fix the issues and run it again until it pa...
(FIX_RESOLVE)
[uncategorized] ~323-~323: Possible missing comma found.
Context: .... DeepSource shell issues: - Use ShellCheck disable comments for specific lines. ...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~323-~323: Possible missing preposition found.
Context: ...rce shell issues**: - Use ShellCheck disable comments for specific lines. - Updat...
(AI_HYDRA_LEO_MISSING_TO)
🪛 Shellcheck (0.10.0)
internal/build/fix_whitespace.sh
[warning] 18-18: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 33-33: This redirection doesn't have a command. Move to its command (or use 'true' as no-op).
(SC2188)
🔇 Additional comments (2)
internal/build/fix_whitespace.sh (1)
38-40:tail -c 1portability check
tail -cis POSIX, yet older BusyBox builds lack it. If Alpine images are part of CI, add a fallback:if ! tail -c 1 "$file" >/dev/null 2>&1; then [ -n "$(dd if="$file" bs=1 count=1 2>/dev/null)" ] && printf '\n' >>"$file" fiMakefile (1)
105-108: Hard dependency onjqwithout guard
Ifjqis missing locally,make tidyfails early. Emit a clearer message and hint:$(JQ): $(error "jq not found – install it or set JQ to an alternative JSON parser")or switch to POSIX
grep/sedpipeline for simple extraction.
wouldn't the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1121 @amery You're absolutely right! Looking at the current implementation in the Makefile, the The current tool detection pattern: ifeq ($(shell $(PNPX) markdownlint-cli --version 2>&1 | grep -q '^[0-9]' && echo yes),yes)Works as follows when pnpx is missing:
The
So the stderr pollution is effectively silenced, and the build system gracefully degrades to using 🧠 Learnings used |
amery
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/build/fix_whitespace.sh (1)
24-24:sed -iis still GNU-specific – portability issue already raised
Usingsed -iwithout an explicit backup suffix breaks on BSD/macOS sed. Please adopt a portable inplace edit (e.g.sed -e … >"$file.tmp" && mv "$file.tmp" "$file"orsed -i '' …) to keep the script cross-platform.
🧹 Nitpick comments (3)
internal/build/fix_whitespace.sh (2)
18-18: Unused variablesize– remove to avoid dead-code noise
sizeis declared but never read. Shell-check already flags this (SC2034).-local file="$1" last_byte size +local file="$1" last_byte
37-37: Quote arithmetic operand to silence SC2046
Wrap the command substitution so word-splitting can’t occur (tiny but keeps static analysis clean).-elif [ $(wc -c < "$file") -eq 1 ]; then +elif [ "$(wc -c <"$file")" -eq 1 ]; thenAGENT.md (1)
254-259: Inconsistent bullet-point punctuation
For consistency with earlier bullets, end each item with a full stop:-When editing Markdown files, ensure compliance with: +When editing Markdown files, ensure compliance with:…and add periods to the sub-bullets if missing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/build.yml(0 hunks).github/workflows/codecov.yml(1 hunks).github/workflows/test.yml(1 hunks).gitignore(1 hunks).vscode/settings.json(1 hunks)AGENT.md(5 hunks)Makefile(4 hunks)internal/build/cspell.json(1 hunks)internal/build/fix_whitespace.sh(1 hunks)internal/build/languagetool.cfg(1 hunks)internal/build/make_coverage.sh(1 hunks)internal/build/markdownlint.json(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/build.yml
✅ Files skipped from review due to trivial changes (2)
- .vscode/settings.json
- .github/workflows/test.yml
🚧 Files skipped from review as they are similar to previous changes (6)
- .gitignore
- .github/workflows/codecov.yml
- internal/build/languagetool.cfg
- internal/build/cspell.json
- internal/build/markdownlint.json
- internal/build/make_coverage.sh
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
AGENT.md (3)
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: karasz
PR: darvaza-proxy/core#74
File: internal/build/get_version.sh:21-30
Timestamp: 2024-10-01T08:39:29.235Z
Learning: In `get_version.sh`, focus on addressing necessary changes, such as Shellcheck warnings, and avoid suggesting optional readability improvements or stylistic changes unless they address critical issues.
Makefile (3)
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: karasz
PR: darvaza-proxy/core#74
File: internal/build/get_version.sh:21-30
Timestamp: 2024-10-01T08:39:29.235Z
Learning: In `get_version.sh`, focus on addressing necessary changes, such as Shellcheck warnings, and avoid suggesting optional readability improvements or stylistic changes unless they address critical issues.
internal/build/fix_whitespace.sh (5)
Learnt from: karasz
PR: darvaza-proxy/core#74
File: internal/build/get_version.sh:21-30
Timestamp: 2024-10-01T08:39:29.235Z
Learning: In `get_version.sh`, focus on addressing necessary changes, such as Shellcheck warnings, and avoid suggesting optional readability improvements or stylistic changes unless they address critical issues.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#75
File: internal/build/get_version.sh:0-0
Timestamp: 2024-10-08T15:33:35.792Z
Learning: In this project, the use of the `local` keyword and `-a` in the `test` command is acceptable in shell scripts, and POSIX compliance is not a concern.
Learnt from: amery
PR: darvaza-proxy/core#75
File: internal/build/get_version.sh:0-0
Timestamp: 2024-10-01T15:48:48.786Z
Learning: In this project, the use of the `local` keyword and `-a` in the `test` command is acceptable in shell scripts, and POSIX compliance is not a concern.
🪛 LanguageTool
AGENT.md
[style] ~306-~306: Consider using a different verb for a more formal wording.
Context: ...markdownlint - If make tidy fails, fix the issues and run it again until it pa...
(FIX_RESOLVE)
[uncategorized] ~323-~323: Possible missing comma found.
Context: .... DeepSource shell issues: - Use ShellCheck disable comments for specific lines. ...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~323-~323: Possible missing preposition found.
Context: ...rce shell issues**: - Use ShellCheck disable comments for specific lines. - Updat...
(AI_HYDRA_LEO_MISSING_TO)
🪛 Shellcheck (0.10.0)
internal/build/fix_whitespace.sh
[warning] 18-18: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 18-18: size appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 37-37: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (1)
Makefile (1)
59-76:FIX_WHITESPACE_EXCLUDEyields an initial-o, breaking the find expression
When the first exclusion list expands empty ($(FIX_WHITESPACE_EXCLUDE_GO)can be blank), the generated expression starts with-o, makingfindfail. Guard with$(strip …)or prepend a dummy always-false test.
amery
force-pushed
the
pr-amery-buildsys
branch
2 times, most recently
from
86b2cbf to
d0aba20
Compare
…ndling Add a new build tool that automatically removes trailing whitespace and ensures files end with a newline. The script supports both find-based searching and explicit file arguments, with automatic exclusion of binary files and version control directories. Integrated into the make fmt target to handle all non-Go text files, while Go files continue to be processed by gofmt. Signed-off-by: Alejandro Mery <amery@apptly.co>
Add markdownlint-cli support with comprehensive configuration for enforcing consistent markdown formatting. The configuration includes: - 80 character line length limits - Strict formatting rules for headings, lists, and code blocks - Selective HTML element allowlist for necessary formatting - Automatic fixes during make fmt Integration features: - Automatic detection of markdownlint-cli via pnpx - File discovery with vendor/git exclusion - Conditional execution only when tool is available Signed-off-by: Alejandro Mery <amery@apptly.co>
Add CSpell integration for automated spell checking of both Markdown documentation and Go source code comments. Configuration includes: - British English language checking - Custom word list for project-specific terminology - Integration with make tidy for automated checking - Support for checking both .md and .go files The check-spelling target runs conditionally when CSpell is available via pnpx, providing comprehensive spell checking without requiring tool installation. Signed-off-by: Alejandro Mery <amery@apptly.co>
Add LanguageTool integration for automated grammar and style checking of both markdown documentation and Go code comments. Configuration includes: - British English language checking - Disabled rules that conflict with technical documentation - Enabled rules for common grammar issues - Integration with make tidy for automated checking The check-grammar target runs conditionally when LanguageTool is available via pnpx, providing non-intrusive quality checks without requiring tool installation. Signed-off-by: Alejandro Mery <amery@apptly.co>
- Add make_coverage.sh script that runs tests with coverage flags - Add 'make coverage' target to generate coverage reports - Update .gitignore to exclude coverage artifacts - Update clean target to remove coverage files Signed-off-by: Alejandro Mery <amery@apptly.co>
- Add dedicated test.yml workflow for race detection and multi-Go testing - Remove test step from build.yml to avoid duplication - Apply same branch restrictions and PR conditions as build workflow This resolves the -race and -coverprofile incompatibility by separating concerns: build workflow focuses on compilation, test workflow handles race detection and compatibility testing across Go versions. Signed-off-by: Alejandro Mery <amery@apptly.co>
Add GitHub Actions workflow to run tests with coverage and upload results to Codecov. The workflow: - Runs on push to main and pull requests - Uses Go 1.23 - Executes make coverage with atomic coverage mode - Uploads coverage.out to Codecov for tracking Coverage collection uses -covermode=atomic without race detection to avoid incompatibility between -race and -coverprofile flags. Signed-off-by: Alejandro Mery <amery@apptly.co>
- Add DeepSource configuration troubleshooting section - Add GitHub Actions and build tools documentation - Emphasize running 'make tidy' before commits - Add common issues and solutions for linting tools Signed-off-by: Alejandro Mery <amery@apptly.co>
…mples - Add benchmarking, short tests, and CPU count examples - Document integration with make coverage target - Provide targeted coverage analysis examples - Clarify GOTEST_FLAGS as alternative to make coverage for fine control - Add HTML coverage generation example The enhanced documentation helps developers understand when to use GOTEST_FLAGS versus make coverage and provides more practical examples for common testing scenarios. Signed-off-by: Alejandro Mery <amery@apptly.co>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/build/fix_whitespace.sh (1)
23-25:sed -istill not portablePrevious review (see history) flagged this GNU-only invocation. BSD sed (macOS) requires a backup suffix:
sed -i ''. Please switch to a portable temp-file pattern or vendorgsed.
🧹 Nitpick comments (1)
AGENT.md (1)
254-259: Minor consistency nit – bullet list wordingLine 256 starts with “LanguageTool” while the following bullets use an en-dash and lower-case verbs; consider aligning all three bullets for uniform style.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/build.yml(0 hunks).github/workflows/codecov.yml(1 hunks).github/workflows/test.yml(1 hunks).gitignore(1 hunks).vscode/settings.json(1 hunks)AGENT.md(5 hunks)Makefile(4 hunks)internal/build/cspell.json(1 hunks)internal/build/fix_whitespace.sh(1 hunks)internal/build/languagetool.cfg(1 hunks)internal/build/make_coverage.sh(1 hunks)internal/build/markdownlint.json(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/build.yml
🚧 Files skipped from review as they are similar to previous changes (9)
- .vscode/settings.json
- .gitignore
- internal/build/cspell.json
- internal/build/languagetool.cfg
- .github/workflows/test.yml
- internal/build/markdownlint.json
- .github/workflows/codecov.yml
- internal/build/make_coverage.sh
- Makefile
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
AGENT.md (3)
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: karasz
PR: darvaza-proxy/core#74
File: internal/build/get_version.sh:21-30
Timestamp: 2024-10-01T08:39:29.235Z
Learning: In `get_version.sh`, focus on addressing necessary changes, such as Shellcheck warnings, and avoid suggesting optional readability improvements or stylistic changes unless they address critical issues.
internal/build/fix_whitespace.sh (5)
Learnt from: karasz
PR: darvaza-proxy/core#74
File: internal/build/get_version.sh:21-30
Timestamp: 2024-10-01T08:39:29.235Z
Learning: In `get_version.sh`, focus on addressing necessary changes, such as Shellcheck warnings, and avoid suggesting optional readability improvements or stylistic changes unless they address critical issues.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-08T15:33:35.792Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#79
File: internal/build/gen_mk.sh:161-169
Timestamp: 2024-10-01T19:43:25.764Z
Learning: When suggesting refactoring of build scripts, consider the possibility of nested `go.mod` files within subdirectories like `./cmd/`, and ensure that the proposed changes handle nested modules correctly.
Learnt from: amery
PR: darvaza-proxy/core#75
File: internal/build/get_version.sh:0-0
Timestamp: 2024-10-08T15:33:35.792Z
Learning: In this project, the use of the `local` keyword and `-a` in the `test` command is acceptable in shell scripts, and POSIX compliance is not a concern.
Learnt from: amery
PR: darvaza-proxy/core#75
File: internal/build/get_version.sh:0-0
Timestamp: 2024-10-01T15:48:48.786Z
Learning: In this project, the use of the `local` keyword and `-a` in the `test` command is acceptable in shell scripts, and POSIX compliance is not a concern.
🪛 LanguageTool
AGENT.md
[style] ~306-~306: Consider using a different verb for a more formal wording.
Context: ...markdownlint - If make tidy fails, fix the issues and run it again until it pa...
(FIX_RESOLVE)
[uncategorized] ~323-~323: Possible missing comma found.
Context: .... DeepSource shell issues: - Use ShellCheck disable comments for specific lines. ...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~323-~323: Possible missing preposition found.
Context: ...rce shell issues**: - Use ShellCheck disable comments for specific lines. - Updat...
(AI_HYDRA_LEO_MISSING_TO)
🪛 Shellcheck (0.10.0)
internal/build/fix_whitespace.sh
[warning] 18-18: In POSIX sh, 'local' is undefined.
(SC3043)
karasz
left a comment
karasz
left a comment
There was a problem hiding this comment.
LGTM, ads a lot of polish to core
Summary
Changes
Build System Enhancements
make coveragetarget that runs tests with coverage flags and generates reportsmake check-spellingmake check-grammarmake fmtfix_whitespace.shscript that removes trailing whitespace and ensures proper EOFCI/CD Integration
Configuration Files
internal/build/cspell.json: British English spell checking configuration with project-specific dictionaryinternal/build/languagetool.cfg: Grammar checking rules for missing articles, punctuation, and hyphenationinternal/build/markdownlint.json: Markdown formatting rules with 80-character line limits and additional consistency rulesDocumentation Updates
Test Plan
make coveragegenerates coverage reports correctlymake check-spellingdetects spelling errorsmake check-grammardetects grammar issuesmake fmtfixes whitespace and Markdown formattingmake tidySummary by CodeRabbit
New Features
Documentation
Chores