Prechádzať zdrojové kódy

name the file a delete removed, and keep delete a name

`delete` prints what `save` prints, in the same words: the profile and the file
it removed, the file annotated as the user's when that is where it was. A
command that removes something and says nothing cannot be told apart from one
that removed something else, and both lines are in one shape now, next to each
other, so they cannot drift.

The reporting must not make the command more capable than it was. `delete` is
name-only, so `delete lava.json` names the profile `lava-json` and not a file,
and DeleteProfile returning the path it removed is how the command can say so
without resolving a second time. The name-to-file mapping is now profileFilePath,
the name form of resolveProfileTarget on its own, and both `Save` and
`DeleteProfile` go through it — a name resolves to one file in one function
rather than by a filepath.Join repeated at each use.

A test holds the line to that: a profile in the user's directory and a
same-named file in the working directory, and after `delete lava` the first is
gone, the second is still there, the line names the first, and `delete
lava.json` refuses rather than removing either.

Also drops a `var name string` in the command that only ever held the argument.
Paul Klumpp 1 týždeň pred
rodič
commit
8fbcf00f0c

+ 4 - 2
.claude/skills/qmk-rgb/SKILL.md

@@ -63,7 +63,9 @@ Profiles are stored as JSON in the per-user `profiles/` directory
 and written to, so a `save` is a `load` away. A checkout's own `profiles/`
 directory is not searched for a name; an argument ending in `.json` is a path, and
 that file is read or written where it says, so `load profiles/lava.json` uses a
-file in the working directory. They persist RGB state across reboots.
+file in the working directory. `delete` is name-only, so a file in a checkout is
+removed with `rm` rather than with this tool. `save` and `delete` both report the
+file they wrote or removed. They persist RGB state across reboots.
 
 ```bash
 qmk-rgb-tool list                    # List saved profiles
@@ -72,7 +74,7 @@ qmk-rgb-tool save profiles/lava.json # Save to that file instead
 qmk-rgb-tool load name               # Apply a profile to every channel it names
 qmk-rgb-tool load name side          # Apply it to one channel or a list of them only
 qmk-rgb-tool load profiles/lava.json # Apply the profile in that file
-qmk-rgb-tool delete name             # Remove a profile, by name only
+qmk-rgb-tool delete name             # Remove a profile, by name only, and report it
 ```
 
 ## Setting up a keyboard whose effect names are missing

+ 6 - 1
AGENTS.md

@@ -341,7 +341,12 @@ exists for. Three shapes are deliberately
 not in that list: `effect` with no effect name routes to the effect list,
 `keyboard fetch` writes a file and prints a line about it, and `save` writes a
 file and says which one — the second only since `save` could name a file by path,
-where a bare "saved" would leave the one thing worth reporting unsaid.
+where a bare "saved" would leave the one thing worth reporting unsaid. `delete`
+writes no file and says which one it removed, for the same reason: a command that
+removes something and prints nothing cannot be told apart from one that removed
+something else. The three lines come in one shape, `savedProfileLine` and
+`deletedProfileLine` next to each other in profile.go, so a file the user asked
+for is always named back to them and annotated by describeDataDir.
 
 `keyboard info` does not open the board, so it must not report the board's
 channels or an effect list from one: it reports the name and whether this tool has

+ 4 - 2
README.md

@@ -71,7 +71,9 @@ without regard to case and the path is handed to the file system as written, so
 `load lala.json` looks for `./lala.json` and says so when it is not there, rather
 than resolving `lala.json` to a profile named `lala-json`. There is no fallback
 from a path to a name, so `list` and the shell completion keep listing the names in
-the per-user directory and nothing else.
+the per-user directory and nothing else. `delete` is name-only, so a file outside
+the per-user directory is read and written and never removed; `rm` is the tool for
+that, and `delete` says which file it removed.
 
 The Impact 80's definition is additionally built into the binary and consulted
 last, so an installed tool has it; see
@@ -836,7 +838,7 @@ board", and that file is gone, so the field would have been unanswerable:
 | `qmk-rgb-tool save [name\|file]`      | Save the current state of **every** channel as `<name>.json` in the per-user `profiles/` (name lowercased, non-`[a-z0-9-_]` mapped to `-`, a leading `-` prefixed with `unnamed-`); without a name it writes `default`. An argument ending in `.json` is a path, and that file is written instead, named after the file's own name. It reports the file it wrote, and annotates a path in the per-user directory as such |
 | `qmk-rgb-tool load <name\|file> [zone]` | Load and apply a profile, by name from the per-user `profiles/` or from a path when the argument ends in `.json`; without a zone it applies the profile to every channel it names, with one it applies it to the named channels only |
 | `qmk-rgb-tool list`                   | List saved profiles                        |
-| `qmk-rgb-tool delete [name]`          | Delete a saved profile; without a name it deletes `default` |
+| `qmk-rgb-tool delete [name]`          | Delete a saved profile; without a name it deletes `default`. A name only, so an argument ending in `.json` names the profile without the dots rather than a file to remove. It reports the file it removed |
 | `qmk-rgb-tool completion <shell>`     | Write an autocompletion script for `bash`, `zsh`, `fish` or `powershell` to stdout |
 
 `enable` and `disable` take a zone and nothing else. `brightness`, `speed` and

+ 41 - 13
cmd/qmk-rgb-tool/profile.go

@@ -116,16 +116,25 @@ func profileFileName(name string) string {
 // keeps a .json argument from also resolving to `lala-json.json`.
 func resolveProfileTarget(arg string) (path string, name string, isPath bool) {
 	if !strings.HasSuffix(strings.ToLower(arg), ".json") {
-		return filepath.Join(profilesPath(), profileFileName(arg)), arg, false
+		return profileFilePath(arg), arg, false
 	}
 	base := filepath.Base(arg)
 	return arg, strings.TrimSuffix(base, filepath.Ext(base)), true
 }
 
+// profileFilePath is the file a name maps to, in the per-user directory. It is the
+// name form of resolveProfileTarget on its own, for the commands that take no path
+// at all: `delete` and `list` are name-only, so `delete lava.json` names the
+// profile `lava-json` and not a file, and the one that says which file it removed
+// must not be the one that could remove a file outside the per-user directory.
+func profileFilePath(name string) string {
+	return filepath.Join(profilesPath(), profileFileName(name))
+}
+
 // Save writes the profile into the per-user directory under its own name, which is
 // what every caller that has a name and no path means.
 func (p *Profile) Save() error {
-	return p.saveTo(filepath.Join(profilesPath(), profileFileName(p.Name)))
+	return p.saveTo(profileFilePath(p.Name))
 }
 
 // saveTo writes the profile to one file and creates the directory it is in. It
@@ -207,15 +216,19 @@ func ListProfiles() ([]string, error) {
 	return names, nil
 }
 
-func DeleteProfile(name string) error {
-	path := filepath.Join(profilesPath(), profileFileName(name))
+// DeleteProfile removes the profile of that name from the per-user directory and
+// returns the file it removed, so a command that reports the deletion says which
+// file it was. A name only: nothing here resolves a path, so an argument ending in
+// .json names the profile `lava-json`.
+func DeleteProfile(name string) (string, error) {
+	path := profileFilePath(name)
 	if err := os.Remove(path); err != nil {
 		if os.IsNotExist(err) {
-			return fmt.Errorf("profile %s not found in %s", name, profilesPath())
+			return "", fmt.Errorf("profile %s not found in %s", name, profilesPath())
 		}
-		return fmt.Errorf("delete profile %s: %w", name, err)
+		return "", fmt.Errorf("delete profile %s: %w", name, err)
 	}
-	return nil
+	return path, nil
 }
 
 func sanitizeFilename(name string) string {
@@ -302,6 +315,14 @@ func savedProfileLine(name, path string) string {
 	return fmt.Sprintf("Saved profile %s to %s\n", name, describeDataDir(path))
 }
 
+// deletedProfileLine is what a delete says it did, in the same words as the save:
+// the name and the file it removed, and the file annotated as the user's when that
+// is where it was. A delete that printed nothing is a command that has run and
+// cannot be told apart from one that removed something else.
+func deletedProfileLine(name, path string) string {
+	return fmt.Sprintf("Deleted profile %s from %s\n", name, describeDataDir(path))
+}
+
 func NewProfileSaveCmd() *cobra.Command {
 	return &cobra.Command{
 		Use:   "save [name|file]",
@@ -531,19 +552,26 @@ func NewProfileListCmd() *cobra.Command {
 }
 
 func NewProfileDeleteCmd() *cobra.Command {
-	var name string
 	cmd := &cobra.Command{
 		ValidArgsFunction: completeProfileNames,
 		Use:               "delete [name]",
 		Short:             "Delete a saved profile",
-		Args:              cobra.MaximumNArgs(1),
+		Long: "Delete a profile from the per-user profiles/ directory, by name; without a\n" +
+			"name it deletes `default`. A path is not accepted, so an argument ending in\n" +
+			".json names the profile without the dots rather than a file to remove. It\n" +
+			"reports the file it removed.",
+		Args: cobra.MaximumNArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
-			if len(args) == 0 {
-				name = "default"
-			} else {
+			name := "default"
+			if len(args) > 0 {
 				name = args[0]
 			}
-			return DeleteProfile(name)
+			path, err := DeleteProfile(name)
+			if err != nil {
+				return err
+			}
+			fmt.Fprint(cmd.OutOrStdout(), deletedProfileLine(name, path))
+			return nil
 		},
 	}
 	return cmd

+ 55 - 0
cmd/qmk-rgb-tool/profile_path_test.go

@@ -171,6 +171,61 @@ func TestSaveProfileToWritesThePathItWasGiven(t *testing.T) {
 	}
 }
 
+// A delete says which file it removed, and saying so must not make it resolve a
+// path: `delete` is name-only, so `delete lava.json` names no file, it names the
+// profile `lava-json`. A same-named file in the working directory is the thing
+// that would go if the report were read as permission to touch it.
+func TestDeleteIsNameOnlyAndSaysWhichFileItRemoved(t *testing.T) {
+	userDir := t.TempDir()
+	t.Cleanup(stubUserConfigDir(t, userDir))
+	t.Cleanup(func() { profilesDirOverride = "" })
+	here := t.TempDir()
+	t.Chdir(here)
+
+	inWorkingDir := filepath.Join(here, "lava.json")
+	if err := os.WriteFile(inWorkingDir, []byte(`{"name":"lava","version":1,"zones":{}}`), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	dir := filepath.Join(userDir, "qmk-rgb-tool", "profiles")
+	if err := os.MkdirAll(dir, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	saved := filepath.Join(dir, "lava.json")
+	if err := os.WriteFile(saved, []byte(`{"name":"lava","version":1,"zones":{}}`), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	path, err := DeleteProfile("lava")
+	if err != nil {
+		t.Fatalf("DeleteProfile(lava): %v", err)
+	}
+	if path != saved {
+		t.Errorf("DeleteProfile(lava) = %q, want %q", path, saved)
+	}
+	line := deletedProfileLine("lava", path)
+	if !strings.Contains(line, saved) {
+		t.Errorf("deletedProfileLine() = %q, want the file it removed", line)
+	}
+	if !strings.Contains(line, "your user directory") {
+		t.Errorf("deletedProfileLine() = %q, want a name's file marked as the user's", line)
+	}
+	if _, err := os.Stat(saved); err == nil {
+		t.Error("DeleteProfile(lava) left the profile in place")
+	}
+	if _, err := os.Stat(inWorkingDir); err != nil {
+		t.Errorf("DeleteProfile(lava) removed the working directory's file: %v", err)
+	}
+
+	// The .json suffix is not read as a path by a name-only command: it names the
+	// profile `lava-json`, which is not there.
+	if _, err := DeleteProfile("lava.json"); err == nil {
+		t.Error(`DeleteProfile("lava.json") = nil error, want the name form refused`)
+	}
+	if _, err := os.Stat(inWorkingDir); err != nil {
+		t.Errorf(`DeleteProfile("lava.json") removed the working directory's file: %v`, err)
+	}
+}
+
 // A save says which file it wrote, and the path is annotated the way every other
 // message in the tool is: a bare path does not say whether it is the user's
 // directory or one the argument named.