Explorar o código

clear the minor doc/code drift left by the review

effect --list emitted a bare JSON array while every other command emitted an
object, because the EffectList wrapper it already built was bypassed in favour
of its slice. The output now uses the wrapper, so all JSON commands share one
shape and a consumer can add fields without breaking. The dead type is no
longer dead.

README said "32-byte feature reports" while hid.go writes output reports with
report ID 0; it now matches AGENTS.md and the code.

README claimed commands run only when exactly one keyboard is connected.
keyboard info, list and delete never open a keyboard, so that overreached;
the constraint is now scoped to the commands that actually open one.

README described profiles as profiles/<name>.json without saying the name is
transformed: "Work Profile 2026" lands in work-profile-2026.json, since
sanitizeFilename lowercases and maps every character outside [a-z0-9-_] to -.

effect --list was missing from both CLI reference tables even though the qmk-rgb
skill depends on it for discovery. It is now listed in both, alongside the
argument arity of every command.

AGENTS.md claimed gopls was "built into Go toolchain". $(go env GOROOT)/bin
contains go and gofmt only; gopls is installed separately via go install. It
also omitted internal/hid/ from the architecture, though via and device both
import it.

AGENTS.md wrote save/load/delete as <name> while the Use strings and README
correctly use [name]; all three are MaximumNArgs(1) defaulting to "default",
which was undocumented.

Finally, a regression guard for the class of bug this whole review came from:
two tests now read README.md, AGENTS.md and SKILL.md and fail if a doc offers a
path value for --device or uses a non-canonical zone name. The guard matches
path-like values rather than the word "path", so prose explaining why paths are
unstable still passes. Verified it catches the original defect by reintroducing
`--device /dev/hidraw7` into the README.
Paul Klumpp hai 1 semana
pai
achega
e3546e22fc
Modificáronse 5 ficheiros con 121 adicións e 11 borrados
  1. 8 4
      AGENTS.md
  2. 8 6
      README.md
  3. 1 1
      cmd/qmk-rgb-tool/effect.go
  4. 51 0
      cmd/qmk-rgb-tool/effect_list_test.go
  5. 53 0
      cmd/qmk-rgb-tool/flags_test.go

+ 8 - 4
AGENTS.md

@@ -8,6 +8,7 @@ Cross-platform Go CLI library for programmatic/agent-friendly control of QMK key
 ```
 cmd/                    # Cobra-based CLI entrypoints
 internal/device/        # QMK Raw HID discovery and keyboard numbering
+internal/hid/           # Cross-platform HID access
 internal/via/           # VIA protocol implementation + LED subsystem
 internal/rgb/           # Effects, values, color definitions
 keyboards.json          # Optional name/metadata lookup for known keyboards
@@ -20,6 +21,7 @@ qmk-rgb-tool keyboard info
 qmk-rgb-tool effect breathing
 qmk-rgb-tool effect rainbow_moving_chevron
 qmk-rgb-tool effect rainbow_moving_chevron --zone backlight
+qmk-rgb-tool effect --list            # Every effect per zone (JSON)
 qmk-rgb-tool brightness <val>       # 0-255, verified by read-back
 qmk-rgb-tool speed <val>            # 0-255, verified by read-back
 qmk-rgb-tool color <hex>            # Six hexadecimal digits
@@ -27,13 +29,15 @@ qmk-rgb-tool mode <index>           # Raw zone-specific effect ID
 qmk-rgb-tool enable
 qmk-rgb-tool disable
 qmk-rgb-tool info
-qmk-rgb-tool save <name>        # Save current RGB state to a profile
-qmk-rgb-tool load <name>        # Load a profile and apply it to the keyboard
-qmk-rgb-tool delete <name>      # Delete a saved profile
+qmk-rgb-tool save [name]        # Save current RGB state to a profile
+qmk-rgb-tool load [name]        # Load a profile and apply it to the keyboard
+qmk-rgb-tool delete [name]      # Delete a saved profile
 qmk-rgb-tool list               # List saved profiles
 qmk-rgb-tool --device <n> ...   # Target keyboard number from `keyboard info`
 ```
 
+`save`, `load` and `delete` take an optional name and default to `default`.
+
 ## Device Selection
 
 Discovery matches every connected keyboard exposing the QMK Raw HID signature
@@ -167,7 +171,7 @@ Do not silently pick a winner — explicitly state the conflict and get directio
 
 ### Tooling
 
-- `gopls` — Go Language Server, built into Go toolchain (LSP, diagnostics, go-to-def, rename, references)
+- `gopls` — Go Language Server, installed separately with `go install golang.org/x/tools/gopls@latest` (LSP, diagnostics, go-to-def, rename, references)
 - `gofmt` / `goimports` — formatting. `goimports` adds/removes imports automatically. Always run before committing.
 - `go test` — built-in test framework. Tests live in `*_test.go` files alongside source.
 - `go vet` — static analysis. Run before committing.

+ 8 - 6
README.md

@@ -76,9 +76,10 @@ All output is machine-parseable JSON when applicable.
 ./qmk-rgb-tool --device 2 brightness 160
 ```
 
-Without `--device`, commands run only when exactly one keyboard is connected.
-With two or more, they fail and list the numbers you can choose from, so a
-command never hits an unintended keyboard.
+Without `--device`, commands that open a keyboard run only when exactly one is
+connected. With two or more, they fail and list the numbers you can choose from,
+so a command never hits an unintended keyboard. `keyboard info`, `list` and
+`delete` never open a keyboard and work with any number of them.
 
 Numbers are assigned in a stable order (vendor ID, product ID, path), but they
 are only guaranteed for the current session. Keyboards are identified by their
@@ -282,7 +283,7 @@ Two models are listed in `keyboards.json`:
 | Keyboard        | VID    | PID    |
 |-----------------|--------|--------|
 | Wobkey Rainy 75 | 0x6666 | 0x0001 |
-| Wobkey Impact 80| 0x36B0 | 0x309F |
+| Wobkey Impact 80 | 0x36B0 | 0x309F |
 
 ## CLI Reference
 
@@ -293,13 +294,14 @@ Two models are listed in `keyboards.json`:
 | `qmk-rgb-tool disable`                | Disable selected lighting zones           |
 | `qmk-rgb-tool info`                   | Show per-zone RGB state (JSON)            |
 | `qmk-rgb-tool effect <name>`          | Set a zone-aware effect by name           |
+| `qmk-rgb-tool effect --list`          | List every effect per zone (JSON)         |
 | `qmk-rgb-tool brightness <val>`       | Set brightness (0–255) on selected zones, verified by read-back |
 | `qmk-rgb-tool speed <val>`            | Set effect speed (0–255) on selected zones, verified by read-back |
 | `qmk-rgb-tool color <hex>`            | Set color (e.g. `ff0000`) on selected zones |
 | `qmk-rgb-tool mode <index>`           | Set a raw zone-specific effect ID         |
 | `qmk-rgb-tool --zone <zone> ...`      | Target `logo`, `backlight`, or `side`     |
 | `qmk-rgb-tool --device <n> ...`      | Target keyboard by number (see `keyboard info`) |
-| `qmk-rgb-tool save [name]`            | Save current RGB state to `profiles/<name>.json` |
+| `qmk-rgb-tool save [name]`            | Save current RGB state to `profiles/<name>.json` (name lowercased, non-`[a-z0-9-_]` mapped to `-`) |
 | `qmk-rgb-tool load [name]`            | Load and apply a profile from `profiles/` |
 | `qmk-rgb-tool list`                   | List saved profiles                        |
 | `qmk-rgb-tool delete [name]`          | Delete a saved profile                     |
@@ -328,7 +330,7 @@ internal/via/     # VIA protocol implementation
 
 ## Protocol
 
-Communicates via the QMK Raw HID interface (`Usage Page 0xFF60`, `Usage 0x61`) with 32-byte feature reports.
+Communicates via the QMK Raw HID interface (`Usage Page 0xFF60`, `Usage 0x61`) with 32-byte reports, report number `0`.
 
 - `0x07` — Custom set value
 - `0x08` — Custom get value

+ 1 - 1
cmd/qmk-rgb-tool/effect.go

@@ -57,7 +57,7 @@ func listAllEffects(cmd *cobra.Command) error {
 		}
 	}
 
-	return encodeJSON(cmd.OutOrStdout(), list.Zones)
+	return encodeJSON(cmd.OutOrStdout(), list)
 }
 
 func runEffectSet(cmd *cobra.Command, args []string) error {

+ 51 - 0
cmd/qmk-rgb-tool/effect_list_test.go

@@ -0,0 +1,51 @@
+package main
+
+import (
+	"bytes"
+	"encoding/json"
+	"testing"
+)
+
+// Every other JSON command emits an object, so a consumer can add fields
+// later without breaking. `effect --list` returned a bare array because the
+// EffectList wrapper it built was bypassed in favour of its slice.
+func TestEffectListEmitsAnObjectNotABareArray(t *testing.T) {
+	var out, errOut bytes.Buffer
+	cmd := NewEffectCmd()
+	cmd.SetOut(&out)
+	cmd.SetErr(&errOut)
+	cmd.SetArgs([]string{"--list"})
+
+	if err := cmd.Execute(); err != nil {
+		t.Fatalf("effect --list returned error: %v", err)
+	}
+
+	var parsed map[string][]struct {
+		Zone   string `json:"zone"`
+		Effect string `json:"effect"`
+		ID     int    `json:"id"`
+	}
+	if err := json.Unmarshal(out.Bytes(), &parsed); err != nil {
+		t.Fatalf("effect --list output is not a JSON object: %v (output %q)", err, out.String()[:min(80, out.Len())])
+	}
+
+	zones, ok := parsed["zones"]
+	if !ok {
+		t.Fatalf("output = %q, want a top-level \"zones\" key", out.String()[:min(80, out.Len())])
+	}
+	if len(zones) == 0 {
+		t.Fatal("zones is empty, want the full catalog")
+	}
+
+	// The catalog itself must not have changed shape.
+	if zones[0].Zone == "" || zones[0].Effect == "" {
+		t.Errorf("zones[0] = %+v, want zone, effect and id populated", zones[0])
+	}
+}
+
+func min(a, b int) int {
+	if a < b {
+		return a
+	}
+	return b
+}

+ 53 - 0
cmd/qmk-rgb-tool/flags_test.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"os"
 	"strings"
 	"testing"
 )
@@ -37,6 +38,58 @@ func TestFlagUsageHasNoValuePlaceholder(t *testing.T) {
 	}
 }
 
+// The flag usage is not the only place a user learns what --device takes. The
+// same promise is repeated in three documentation files, and it was wrong in
+// all of them at once. Pin the docs to the flag's actual contract by looking
+// for a value that looks like a HID path, not for the word "path" — prose
+// about why paths are unstable, and the rule that forbids them, must pass.
+func TestDocsDoNotOfferAPathValueForDevice(t *testing.T) {
+	pathLike := []string{"/dev/", "hidraw", `\\?\hid`, "IO/HIDDevice"}
+
+	for _, path := range []string{
+		"../../README.md",
+		"../../AGENTS.md",
+		"../../.claude/skills/qmk-rgb/SKILL.md",
+	} {
+		data, err := os.ReadFile(path)
+		if err != nil {
+			t.Errorf("read %s: %v", path, err)
+			continue
+		}
+		for _, line := range strings.Split(string(data), "\n") {
+			if !strings.Contains(line, "--device") {
+				continue
+			}
+			for _, needle := range pathLike {
+				if strings.Contains(line, needle) {
+					t.Errorf("%s offers a path value for --device: %q", path, strings.TrimSpace(line))
+				}
+			}
+		}
+	}
+}
+
+// The zone names are a three-way contract between the resolver and the docs.
+func TestDocsUseTheCanonicalZoneNames(t *testing.T) {
+	for _, path := range []string{
+		"../../README.md",
+		"../../AGENTS.md",
+		"../../.claude/skills/qmk-rgb/SKILL.md",
+	} {
+		data, err := os.ReadFile(path)
+		if err != nil {
+			t.Errorf("read %s: %v", path, err)
+			continue
+		}
+		body := string(data)
+		for _, wrong := range []string{"--zone logo|backlight", "--zone matrix", "zone=matrix"} {
+			if strings.Contains(body, wrong) {
+				t.Errorf("%s contains %q, want only the canonical zone names", path, wrong)
+			}
+		}
+	}
+}
+
 func TestZoneFlagUsageListsEveryZone(t *testing.T) {
 	flag := newRootCommand().PersistentFlags().Lookup("zone")
 	if flag == nil {