ci: fix lint, markdown links, and build #282
Conversation
WalkthroughThe pull request introduces various changes to the workflow configurations and documentation for the Go project. The Changes
Assessment against linked issues
Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #282 +/- ##
==========================================
+ Coverage 68.01% 69.54% +1.52%
==========================================
Files 6 6
Lines 1138 1484 +346
==========================================
+ Hits 774 1032 +258
- Misses 337 425 +88
Partials 27 27 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
Makefile (2)
1-7: Consider enhancing the lint target with additional features.The lint target implementation is clean and well-documented. However, consider these improvements:
- Add checks for tool availability
- Allow running individual linters via sub-targets
- Make the markdown file pattern configurable
+MARKDOWN_FILES ?= '**/*.md' + ## lint: Run all linters; golangci-lint, markdownlint. -lint: +lint: lint-go lint-markdown + +## lint-go: Run golangci-lint +lint-go: + @command -v golangci-lint >/dev/null 2>&1 || { echo "golangci-lint not installed" >&2; exit 1; } @echo "--> Running golangci-lint" @golangci-lint run + +## lint-markdown: Run markdownlint +lint-markdown: + @command -v markdownlint >/dev/null 2>&1 || { echo "markdownlint not installed" >&2; exit 1; } @echo "--> Running markdownlint" - @markdownlint --config .markdownlint.yaml '**/*.md' - -.PHONY: lint + @markdownlint --config .markdownlint.yaml $(MARKDOWN_FILES) + +.PHONY: lint lint-go lint-markdown
9-13: Enhance markdown-link-check with configuration and performance improvements.The implementation handles filenames safely with
-print0and-0flags. Consider these improvements:
- Add tool availability check
- Add configuration file support
- Add directory exclusions
- Enable parallel processing for faster checks
+MARKDOWN_EXCLUDE_DIRS ?= -not -path "./vendor/*" -not -path "./node_modules/*" + ## markdown-link-check: Check all markdown links. markdown-link-check: + @command -v markdown-link-check >/dev/null 2>&1 || { echo "markdown-link-check not installed" >&2; exit 1; } @echo "--> Running markdown-link-check" - @find . -name \*.md -print0 | xargs -0 -n1 markdown-link-check + @find . -name \*.md $(MARKDOWN_EXCLUDE_DIRS) -print0 | xargs -0 -n1 -P4 markdown-link-check -c .markdown-link-check.jsonAlso, consider creating a
.markdown-link-check.jsonconfiguration file to customize behavior:{ "ignorePatterns": [ { "pattern": "^https?://localhost" } ], "retryOn429": true, "retryCount": 3, "fallbackRetryDelay": "30s" }.github/workflows/lint.yml (1)
25-25: Add a comment explaining version choiceConsider adding a comment explaining why this specific version was chosen. This helps future maintainers understand the rationale behind version pinning decisions.
+ # Pinned to v1.61.0 for stability and reproducible builds version: v1.61.0nmt_quint_model/README.md (2)
Line range hint
84-84: Consider making the hardcoded path reference more explicit.The documentation mentions a hardcoded path in
simulation_test.gobut doesn't specify the actual path. Consider adding the complete path for better clarity.
Line range hint
84-84: Fix unclosed code block.The markdown code block at the end of the file is missing its closing triple backticks.
Add the following at the end of the file:
+ ```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
.github/workflows/go.yml(1 hunks).github/workflows/lint.yml(1 hunks).golangci.yml(1 hunks)Makefile(1 hunks)README.md(0 hunks)go.mod(1 hunks)nmt_quint_model/README.md(1 hunks)subrootpaths_test.go(0 hunks)
💤 Files with no reviewable changes (2)
- README.md
- subrootpaths_test.go
✅ Files skipped from review due to trivial changes (1)
- go.mod
🧰 Additional context used
🪛 actionlint
.github/workflows/go.yml
32-32: shellcheck reported issue in this script: SC2155:warning:1:8: Declare and assign separately to avoid masking return values
(shellcheck)
🔇 Additional comments (4)
.golangci.yml (1)
7-7: LGTM! Good update to use the recommended linter.
The replacement of the deprecated exportloopref with copyloopvar is the correct approach, as copyloopvar is the modern recommended linter for detecting loop variable reference issues.
Let's verify there are no existing loop-related issues that might be caught by the new linter:
✅ Verification successful
Based on the shell script results, I can see several loop variable usages in the codebase. However, all the identified loops appear to be using variables safely:
- Most loops are using range-based iteration with proper scoping of variables
- The variable declarations inside loops are properly scoped and don't have potential closure issues
- No instances of loop variables being captured in goroutines or closures were found
- The loops primarily deal with test cases, data processing, and tree operations in a safe manner
The switch to copyloopvar linter is appropriate and no issues were found.
The codebase demonstrates good practices in loop variable handling, and the new linter will help maintain this standard going forward.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential loop variable reference issues that the new linter might catch
# Look for range loops with variable declarations that might be problematic
# Search for range loops with variable declarations
ast-grep --pattern 'for $_ := range $_ {
$$$
var $_ $_
$$$
}'
# Search for range loops with short variable declarations
ast-grep --pattern 'for $_ := range $_ {
$$$
$_ := $_
$$$
}'
Length of output: 20505
Makefile (1)
11-12: Verify tool effectiveness for known dead links.
Let's verify if the markdown-link-check tool can detect the specific dead links mentioned in issue #269.
.github/workflows/go.yml (1)
15-17: LGTM! Using latest checkout action version.
The checkout step is properly configured using the latest stable version (v4) of the actions/checkout action.
nmt_quint_model/README.md (1)
64-66: Verify accessibility of the updated documentation links.
The links have been updated to point to the official Quint documentation site instead of the GitHub repository. Let's verify these links are accessible.
✅ Verification successful
All documentation links are accessible and working correctly
All three updated URLs return HTTP 200 status codes, confirming they are valid and accessible:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the updated documentation links are accessible
# Expected: All URLs should return HTTP 200 status code
urls=(
"https://quint-lang.org/docs/lessons"
"https://quint-lang.org/quint-cheatsheet.pdf"
"https://quint-lang.org/docs/builtin"
)
for url in "${urls[@]}"; do
echo "Checking $url"
status_code=$(curl -s -o /dev/null -w "%{http_code}" "$url")
echo "Status code: $status_code"
done
Length of output: 946
| - name: Test | ||
| run: | | ||
| export PATH=$PATH:$(go env GOPATH)/bin | ||
| go test -v -coverpkg=./... -covermode=atomic -coverprofile coverage.txt ./... | ||
|
|
||
| - name: Upload coverage to Codecov | ||
| uses: codecov/codecov-action@v4 | ||
| with: | ||
| token: ${{ secrets.CODECOV_TOKEN }} | ||
| fail_ci_if_error: true | ||
| go test -v ./... |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix PATH export and document coverage removal.
- The current PATH export could mask return values. Let's fix that.
- The removal of code coverage aligns with the PR objectives to simplify the workflow.
Apply this change to fix the PATH export:
- name: Test
run: |
- export PATH=$PATH:$(go env GOPATH)/bin
+ GOPATH_BIN="$(go env GOPATH)/bin"
+ export PATH=$PATH:$GOPATH_BIN
go test -v ./...Consider creating a separate workflow for code coverage reporting if needed in the future. This separation of concerns will make the workflows more maintainable and focused.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Test | |
| run: | | |
| export PATH=$PATH:$(go env GOPATH)/bin | |
| go test -v -coverpkg=./... -covermode=atomic -coverprofile coverage.txt ./... | |
| - name: Upload coverage to Codecov | |
| uses: codecov/codecov-action@v4 | |
| with: | |
| token: ${{ secrets.CODECOV_TOKEN }} | |
| fail_ci_if_error: true | |
| go test -v ./... | |
| - name: Test | |
| run: | | |
| GOPATH_BIN="$(go env GOPATH)/bin" | |
| export PATH=$PATH:$GOPATH_BIN | |
| go test -v ./... |
🧰 Tools
🪛 actionlint
32-32: shellcheck reported issue in this script: SC2155:warning:1:8: Declare and assign separately to avoid masking return values
(shellcheck)
Closes #269, #275, #279, #222 (b/c no longer relevant)
Note: this PR removes code coverage. If we want it back, IMO it should be a separate workflow.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores