Repository navigation
Fixes #5140 - clean code of unused embed with side-effects - #5141
Conversation
WalkthroughThis pull request removes unnecessary blank identifier imports ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.17.0)v3/examples/android/main.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m v3/examples/badge/main.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m v3/examples/contextmenus/main.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m
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.
🧹 Nitpick comments (1)
v3/UNRELEASED_CHANGELOG.md (1)
22-22: Consider tightening the changelog wording and linking the issue.This reads a bit informal; using a direct phrasing with issue reference improves release-note clarity.
✏️ Proposed wording
-- Remove some unnecessary `_ "embed"` imports tidying the code a little +- Remove unnecessary `_ "embed"` imports to tidy the codebase (`#5140`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/UNRELEASED_CHANGELOG.md` at line 22, Update the informal changelog entry "Remove some unnecessary `_ \"embed\"` imports tidying the code a little" to a tighter, release-note style sentence such as "Remove unnecessary `_ \"embed\"` imports" and append a reference to the related issue or PR (e.g. "(`#1234`)" or a full issue/PR URL) so the change is clear and traceable; ensure the `_ "embed"` token is preserved exactly as shown and the change appears in v3/UNRELEASED_CHANGELOG.md.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@v3/UNRELEASED_CHANGELOG.md`:
- Line 22: Update the informal changelog entry "Remove some unnecessary `_
\"embed\"` imports tidying the code a little" to a tighter, release-note style
sentence such as "Remove unnecessary `_ \"embed\"` imports" and append a
reference to the related issue or PR (e.g. "(`#1234`)" or a full issue/PR URL) so
the change is clear and traceable; ensure the `_ "embed"` token is preserved
exactly as shown and the change appears in v3/UNRELEASED_CHANGELOG.md.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6bb10e2a-2500-411b-a37a-963efc9e96cf
📒 Files selected for processing (71)
v3/UNRELEASED_CHANGELOG.mdv3/examples/android/main.gov3/examples/badge-custom/main.gov3/examples/badge/main.gov3/examples/build/main.gov3/examples/clipboard/main.gov3/examples/contextmenus/main.gov3/examples/custom-protocol-example/main.gov3/examples/dev/main.gov3/examples/dialogs/main.gov3/examples/dock/main.gov3/examples/environment/main.gov3/examples/events-bug/main.gov3/examples/events/main.gov3/examples/file-association/main.gov3/examples/frameless/main.gov3/examples/hide-window/main.gov3/examples/ios/main.gov3/examples/keybindings/main.gov3/examples/notifications/main.gov3/examples/plain/main.gov3/examples/raw-message/main.gov3/examples/show-macos-toolbar/main.gov3/examples/systray-basic/main.gov3/examples/systray-custom/main.gov3/examples/video/main.gov3/examples/window-menu/main.gov3/examples/wml/main.gov3/internal/assetserver/build_dev.gov3/internal/commands/appimage_testfiles/main.gov3/internal/commands/build-assets.gov3/internal/commands/version.gov3/internal/generator/collect/events.gov3/internal/generator/testcases/aliases/main.gov3/internal/generator/testcases/complex_expressions/main.gov3/internal/generator/testcases/complex_instantiations/main.gov3/internal/generator/testcases/complex_json/main.gov3/internal/generator/testcases/complex_method/main.gov3/internal/generator/testcases/cyclic_imports/main.gov3/internal/generator/testcases/cyclic_types/main.gov3/internal/generator/testcases/directives/main.gov3/internal/generator/testcases/enum/main.gov3/internal/generator/testcases/enum_from_imported_package/main.gov3/internal/generator/testcases/enum_map_keys/main.gov3/internal/generator/testcases/function_from_imported_package/main.gov3/internal/generator/testcases/function_from_nested_imported_package/main.gov3/internal/generator/testcases/function_multiple_files/greet.gov3/internal/generator/testcases/function_multiple_files/main.gov3/internal/generator/testcases/function_single/main.gov3/internal/generator/testcases/function_single_context/main.gov3/internal/generator/testcases/function_single_internal/main.gov3/internal/generator/testcases/map_keys/main.gov3/internal/generator/testcases/marshalers/main.gov3/internal/generator/testcases/out_of_tree/main.gov3/internal/generator/testcases/struct_literal_multiple/main.gov3/internal/generator/testcases/struct_literal_multiple_files/greet.gov3/internal/generator/testcases/struct_literal_multiple_files/main.gov3/internal/generator/testcases/struct_literal_multiple_other/main.gov3/internal/generator/testcases/struct_literal_non_pointer_single/main.gov3/internal/generator/testcases/struct_literal_single/main.gov3/internal/generator/testcases/variable_single/main.gov3/internal/generator/testcases/variable_single_from_function/main.gov3/internal/generator/testcases/variable_single_from_other_function/main.gov3/internal/templates/_common/main.go.tmplv3/pkg/services/log/log.gov3/test/4769-menu/main.gov3/test/manual/systray/custom-handlers/main.gov3/test/manual/systray/hide-options/main.gov3/test/manual/systray/menu-only/main.gov3/test/manual/systray/window-menu/main.gov3/test/manual/systray/window-only/main.go
💤 Files with no reviewable changes (46)
- v3/examples/hide-window/main.go
- v3/examples/build/main.go
- v3/examples/clipboard/main.go
- v3/examples/dialogs/main.go
- v3/examples/video/main.go
- v3/internal/generator/testcases/enum_from_imported_package/main.go
- v3/examples/keybindings/main.go
- v3/internal/generator/testcases/struct_literal_multiple_other/main.go
- v3/examples/systray-custom/main.go
- v3/internal/generator/testcases/variable_single/main.go
- v3/internal/assetserver/build_dev.go
- v3/internal/generator/testcases/complex_instantiations/main.go
- v3/internal/generator/testcases/struct_literal_multiple_files/main.go
- v3/internal/generator/testcases/function_multiple_files/main.go
- v3/internal/generator/testcases/complex_expressions/main.go
- v3/internal/generator/testcases/complex_method/main.go
- v3/internal/commands/appimage_testfiles/main.go
- v3/internal/generator/collect/events.go
- v3/internal/generator/testcases/cyclic_imports/main.go
- v3/internal/generator/testcases/variable_single_from_other_function/main.go
- v3/internal/generator/testcases/marshalers/main.go
- v3/internal/generator/testcases/cyclic_types/main.go
- v3/examples/show-macos-toolbar/main.go
- v3/examples/systray-basic/main.go
- v3/internal/generator/testcases/function_single/main.go
- v3/internal/generator/testcases/complex_json/main.go
- v3/internal/generator/testcases/directives/main.go
- v3/internal/generator/testcases/struct_literal_single/main.go
- v3/internal/generator/testcases/function_from_nested_imported_package/main.go
- v3/internal/commands/version.go
- v3/internal/generator/testcases/function_multiple_files/greet.go
- v3/internal/generator/testcases/struct_literal_multiple_files/greet.go
- v3/internal/generator/testcases/struct_literal_non_pointer_single/main.go
- v3/internal/generator/testcases/struct_literal_multiple/main.go
- v3/test/manual/systray/custom-handlers/main.go
- v3/internal/generator/testcases/out_of_tree/main.go
- v3/test/manual/systray/window-only/main.go
- v3/test/manual/systray/hide-options/main.go
- v3/test/manual/systray/window-menu/main.go
- v3/internal/generator/testcases/enum_map_keys/main.go
- v3/internal/generator/testcases/function_from_imported_package/main.go
- v3/test/manual/systray/menu-only/main.go
- v3/internal/generator/testcases/map_keys/main.go
- v3/internal/generator/testcases/enum/main.go
- v3/examples/events-bug/main.go
- v3/internal/generator/testcases/aliases/main.go
|
The macOS test failure isn't a build issue — it's a golden file mismatch in the binding generator tests. The test expects line To fix, delete the outdated golden find v3/internal/generator/testdata/output -name 'warnings.log' -delete
cd v3/internal/generator && go test ./...Then commit the regenerated files. |
|
✅ Triaged by Wails PR Reviewer This PR has been reviewed and accepted. Head Ref OID: This comment serves as a signature that this PR has been triaged. Future runs will skip this PR based on the headRefOid. |
leaanthony
left a comment
leaanthony
left a comment
There was a problem hiding this comment.
LGTM — careful removal of redundant _ "embed" imports. Each file that lost the blank import either retains a real "embed" import for its //go:embed directives, or has no embed directives at all. The build-tag-guarded file in internal/assetserver/build_dev.go was also checked and the removal is safe there too. Good cleanup.
Dismissing approval — PR has merge conflicts and CI failures (Windows + macOS Go tests) from April. Needs to be rebased against v3-alpha before it can be merged.
|
The code change itself is correct, but this PR can't be merged as-is:
@symball — please rebase against |
|
Changelog wording noted — the suggestion to tighten "Remove some unnecessary..." to "Remove unnecessary..." and add the issue reference is a good improvement. Applied/accepted as-is since the PR is already approved (LGTM from maintainer). Will pick up in the merge commit message if needed. |
|
Hi @symball — this PR has a merge conflict (most likely in git fetch upstream
git rebase upstream/master
# if there's a conflict in UNRELEASED_CHANGELOG.md, just keep master's version and re-add your entry
git push --force-with-leaseThe change itself is already approved (LGTM) — just needs the rebase to clear the conflict. |
c6a657d to
47e79e1
Compare
… import removal Removing `_ "embed"` from complex_json/main.go and marshalers/main.go shifted line numbers by 1, invalidating the golden warning log snapshots.
|
Fixed the CI failure — pushed directly to the branch. Root cause: Removing
The |
Windows OS timer granularity is ~15ms so time.Sleep(1ms) can actually sleep 15ms, exceeding the 5ms debounce window and causing the timer to fire mid-loop. Raise debounceAfter to 100ms and inter-call sleep to 5ms so the ratio holds even on loaded CI runners.
|
Pushed one more fix — the Root cause: Windows OS timer granularity is ~15ms, so Fix (db8d299): Raised |
Description
Just a small code clean leaving instances where embed used to create []byte representation of single files alone.
Example of import with side-effects not to be touched
Fixes #5140
How Has This Been Tested?
On MacOS, ran the test suite and things passed to the same level
Summary by CodeRabbit