linting improvements and apply formatting fixes #1194
Conversation
Add swag comment formatting check to ensure API documentation comments follow formatting conventions. It's equivalent to running swag fmt
|
Warning Rate limit exceeded@harrisony has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 16 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (20)
WalkthroughThis PR adds a git-blame-ignore entry, enables the swaggo formatter and forbidigo linter in golangci-lint, applies widespread formatting/import/tag/assertion cleanups across tests and packages, and changes HandleWipeInventory to default to no-wipe options when request-body decoding fails instead of returning an error. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Security Recommendations
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/app/api/handlers/v1/v1_ctrl_actions.go (1)
130-139: Behavioral change: decode errors now default to no-wipe instead of returning an error.The error handling has been modified so that when
server.Decodefails (e.g., malformed JSON or missing body), the handler now defaults to safe no-wipe options instead of returning a bad request error. While this provides a safer default for missing bodies, it silently swallows decode errors from malformed JSON, which could mask client bugs.Consider:
- Should malformed JSON be distinguished from a missing body?
- Should decode failures be logged to help diagnose client issues?
🔎 Potential improvement to log decode failures
// Parse options from request body var options WipeInventoryOptions if err := server.Decode(r, &options); err != nil { + log.Debug().Err(err).Msg("failed to decode wipe options, using safe defaults") // If no body provided, use default (false for all) options = WipeInventoryOptions{ WipeLabels: false, WipeLocations: false, WipeMaintenance: false, } }backend/internal/data/repo/repo_wipe_integration_test.go (1)
15-194: Add handler-level tests for the WipeInventory endpoint with invalid/missing request bodies.The handler (
HandleWipeInventory) correctly defaults to no-wipe (all flags false) when the request body is missing or invalid. However, no handler test files exist inbackend/app/api/handlers/. Add tests to verify:
- Missing request body defaults to no-wipe behavior
- Invalid JSON bodies are handled gracefully
- Empty body objects work as expected
Security note: The current handler silently accepts invalid request bodies without error feedback to the client or logging. Consider adding explicit validation with appropriate error responses (e.g., 400 Bad Request) for malformed JSON, as this improves API clarity and helps developers debug integration issues.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.git-blame-ignore-revsbackend/.golangci.ymlbackend/app/api/handlers/v1/v1_ctrl_actions.gobackend/app/api/handlers/v1/v1_ctrl_reporting.gobackend/app/api/handlers/v1/v1_ctrl_user.gobackend/internal/core/services/main_test.gobackend/internal/core/services/service_items_attachments.gobackend/internal/data/migrations/migrations.gobackend/internal/data/migrations/postgres/20250112202302_sync_children.gobackend/internal/data/migrations/sqlite3/20241226183416_sync_children.gobackend/internal/data/repo/main_test.gobackend/internal/data/repo/repo_item_attachments.gobackend/internal/data/repo/repo_item_attachments_test.gobackend/internal/data/repo/repo_item_templates.gobackend/internal/data/repo/repo_items_search_test.gobackend/internal/data/repo/repo_items_test.gobackend/internal/data/repo/repo_wipe_integration_test.gobackend/internal/sys/analytics/analytics.gobackend/internal/sys/config/conf.gobackend/pkgs/textutils/normalize.go
💤 Files with no reviewable changes (1)
- backend/internal/data/migrations/migrations.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-12T19:31:54.055Z
Learnt from: tankerkiller125
Repo: sysadminsmedia/homebox PR: 897
File: backend/app/api/setup.go:25-25
Timestamp: 2025-07-12T19:31:54.055Z
Learning: In backend/app/api/setup.go, the manual conversion of backslashes to forward slashes using strings.ReplaceAll(storageDir, "\\", "/") is intentional because the underlying storage library only accepts forward slashes for file paths and performs its own path conversion internally.
Applied to files:
backend/internal/data/repo/repo_item_attachments.gobackend/internal/data/repo/repo_item_attachments_test.go
🧬 Code graph analysis (2)
backend/internal/data/repo/repo_items_test.go (6)
frontend/lib/api/types/data-contracts.ts (4)
LocationCreate(880-884)LabelCreate(851-860)ItemCreate(605-618)MaintenanceEntryCreate(931-938)backend/internal/data/repo/repo_locations.go (1)
LocationCreate(22-26)backend/internal/data/repo/repo_labels.go (1)
LabelCreate(21-25)backend/internal/data/repo/repo_items.go (1)
ItemCreate(69-80)backend/internal/data/repo/repo_maintenance_entry.go (1)
MaintenanceEntryCreate(24-30)backend/internal/data/types/date.go (1)
DateFromTime(27-30)
backend/internal/data/repo/repo_wipe_integration_test.go (1)
backend/internal/data/types/date.go (1)
DateFromTime(27-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Frontend Tests / Integration Tests PGSQL 15
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 4/4
- GitHub Check: Frontend Tests / Integration Tests PGSQL 16
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 2/4
- GitHub Check: Frontend Tests / Integration Tests PGSQL 17
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 1/4
- GitHub Check: Backend Server Tests / Go
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 3/4
- GitHub Check: Frontend Tests / Integration Tests
- GitHub Check: build (linux/arm64, ubuntu-24.04-arm)
- GitHub Check: build (linux/amd64, ubuntu-latest)
- GitHub Check: build (linux/arm64, ubuntu-24.04-arm)
- GitHub Check: build (linux/amd64, ubuntu-latest)
- GitHub Check: build (linux/amd64, ubuntu-latest)
- GitHub Check: build (linux/arm64, ubuntu-24.04-arm)
🔇 Additional comments (23)
backend/internal/data/repo/repo_items_search_test.go (1)
167-167: LGTM! More idiomatic assertion method.The change from
NotEqual(t, "", ...)toNotEmpty(t, ...)improves code clarity by using a more expressive testify assertion that directly conveys the intent. No functional change to the test logic.backend/internal/data/repo/repo_items_test.go (3)
401-463: LGTM! Formatting improvements enhance readability.The added blank lines appropriately separate logical test setup sections (locations, labels, items, maintenance entries), making the test structure clearer.
464-500: LGTM! Idiomatic assertion style.The switch to
assert.Positive(line 468) is more idiomatic thanassert.Greater(deleted, 0, ...)for checking positive values. The added blank lines improve test readability by separating verification blocks.
502-557: LGTM! Consistent assertion improvements.The use of
assert.Positive(line 535) maintains consistency with the previous test case and follows testify best practices. Formatting improvements are appropriate.backend/internal/sys/analytics/analytics.go (1)
10-11: LGTM! Import formatting improved.The import reordering aligns with standard Go formatting conventions.
backend/internal/data/migrations/postgres/20250112202302_sync_children.go (1)
7-7: LGTM! Import formatting cleanup.The blank line addition follows standard Go import block formatting.
backend/internal/data/repo/main_test.go (1)
9-10: LGTM! Import organization improved.The import reordering properly separates internal packages with a blank line, following Go formatting conventions.
.git-blame-ignore-revs (1)
1-2: Excellent addition! Best practice for formatting commits.Adding the formatting commit to
.git-blame-ignore-revsensures thatgit blameoutput remains meaningful by skipping over this purely stylistic change. Users can enable it withgit config blame.ignoreRevsFile .git-blame-ignore-revs.backend/app/api/handlers/v1/v1_ctrl_reporting.go (1)
4-5: LGTM! Import formatting cleanup.The
net/httpimport is properly positioned and already used in the handler function.backend/internal/data/migrations/sqlite3/20241226183416_sync_children.go (1)
7-7: LGTM! Import formatting cleanup.The blank line addition follows standard Go import block formatting conventions.
backend/pkgs/textutils/normalize.go (1)
25-31: LGTM! Whitespace cleanup.The removal of extraneous blank lines improves code consistency without affecting functionality.
backend/.golangci.yml (1)
75-75: LGTM! Swaggo formatter enabled.Adding the
swaggoformatter ensures consistent formatting of Swagger/OpenAPI doc comments across the codebase, aligning with the PR objective.backend/app/api/handlers/v1/v1_ctrl_user.go (1)
23-23: LGTM! Swagger documentation formatting improved.The @failure annotation formatting has been aligned with the swaggo formatter standards.
backend/internal/core/services/service_items_attachments.go (1)
5-6: LGTM! Import organization improved.The
ioimport has been properly organized with standard library imports.backend/internal/data/repo/repo_item_attachments.go (1)
113-129: LGTM! Code readability improved.The blank lines enhance the logical grouping within the
fullPathfunction without altering behavior.backend/internal/data/repo/repo_item_attachments_test.go (1)
286-339: LGTM! Test readability improved.The blank lines enhance the visual separation between test sections without changing test behavior.
backend/internal/core/services/main_test.go (1)
9-10: LGTM! Import organization improved.The config import has been properly positioned with a blank line for better grouping.
backend/internal/data/repo/repo_item_templates.go (1)
40-95: LGTM! Struct tag formatting standardized.The struct tag alignment improves readability while maintaining the same validation rules and extensions.
backend/internal/sys/config/conf.go (1)
67-98: LGTM! Configuration struct tag formatting improved.The struct tag alignment in
LabelMakerConfandOIDCConfenhances readability while preserving all default values and configuration behavior.backend/app/api/handlers/v1/v1_ctrl_actions.go (2)
113-114: LGTM! Documentation updated to reflect optional body.The Swagger documentation now correctly indicates that the
optionsparameter is optional, aligning with the new behavior.
122-156: LGTM! Formatting and structure improved.The blank lines enhance readability by visually separating the permission checks, body parsing, action execution, and event publishing sections.
backend/internal/data/repo/repo_wipe_integration_test.go (2)
24-190: LGTM: Formatting improvements enhance readability.The blank line additions throughout the test improve visual separation between logical test steps and are consistent with Go formatting best practices.
111-111: LGTM: Assertion refactors improve test clarity.The changes to use
assert.Positive(instead ofassert.Greater(..., 0)) andassert.Empty(instead ofassert.Equal(0, len(...))) are more idiomatic and clearly express the test intent.Also applies to: 117-117, 173-173
The linter was automatically fixing issues before reporting them, which caused confusing behavior Running golangci-lint would show 0 issues but modify many files. Issues were fixed during the lint run before the final report. In CI, this gave the false impression that everything was fine since no issues were reported, while the codebase was silently being modified. Disabling fix: true ensures issues are reported without being silently fixed
|
@coderabbitai I don't know what you're talking about re this comment "HandleWipeInventory changes decode error handling to default WipeInventoryOptions to false instead of propagating error." it's just formatting and comment changes |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 36 🏁 Script executed: Length of output: 4008 @harrisony You're absolutely right, and I apologize for the incorrect summary. Looking at the actual diff, the changes to
The error handling code that defaults The AI summary incorrectly interpreted this as a behavioral change when it was not. Thank you for catching this error! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2217 @harrisony You're absolutely right, and I apologize for the incorrect analysis. Looking at the actual diff, the changes to
The error handling code with the default Thank you for catching this error! The changes in this file are indeed just formatting and comment alignment fixes, consistent with the PR's objective of applying linter formatting. |
This enforces proper error handling by requiring errors to be returned instead of terminating the application. Ref: sysadminsmedia#953
harrisony
force-pushed
the
features/lint-swag
branch
from
d09c903 to
455f5eb
Compare
harrisony
changed the title
|
@coderabbitai full review @coderabbitai summary |
|
@harrisony I'll conduct a full review of the PR and regenerate the summary for you. ✅ Actions performedSummary regeneration triggered. |
1 participant
What type of PR is this?
What this PR does / why we need it:
I've also thrown in a git-blame-ignore-revs which can be enabled with
git config blame.ignoreRevsFile .git-blame-ignore-revsWhich issue(s) this PR fixes:
None, this was done under my own initiative
Special notes for your reviewer:
Not all of these changes are the result of the addition of the swag formatter, many seemingly missed the standard go fmt of golangci-lint when they were written at the time
Testing
I ran it, the changes look ok
Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.