fix: set-default no longer fails when obsidian.json is missing or unparseable#89
fix: set-default no longer fails when obsidian.json is missing or unparseable#89TriplEight wants to merge 6 commits into
Conversation
…arseable Two fixes: - vault_path.go: if vault name is an absolute path, return it directly without reading obsidian.json (handles the common case of storing full paths as the default vault) - set_default.go: path resolution after setting the name is now best-effort; a warning is printed instead of a fatal error, so the command always succeeds as long as writing preferences.json works
fix: set-default no longer fails when obsidian.json is missing or unparseable
getPathForVault matches by strings.HasSuffix(path, name), so vault
names must be a suffix of the configured path. The original tests used
arbitrary keys ('windows', 'secondary', 'linux') that never matched,
causing all three WSL tests to fail since PR Yakitrak#84.
|
Thank you for the PR, I will take a look soon! |
Yakitrak
left a comment
There was a problem hiding this comment.
Hey, thanks for the PR! The two main fixes make sense - the absolute path shortcut is a nice touch and making path resolution non-fatal is much better UX.
A few things worth addressing before merging:
WSL tests are still failing
The description mentions fixing the three WSL tests, but they still fail on main. The problem is that getPathForVault uses strings.HasSuffix(element.Path, name), so the vault name needs to actually be a suffix of the path. Names like "windows" and "secondary" are just internal Obsidian JSON keys and will never match the path.
The fix is to use the actual folder names instead:
Name: "windows"->Name: "Obsidian Vault"Name: "secondary"->Name: "MyVault"Name: "linux"->Name: "Obsidian Vault"
log.Printf vs fmt.Println in set_default.go
log.Printf writes to stderr and adds a timestamp prefix by default, which will look odd next to the fmt.Println calls. Something like fmt.Fprintln(os.Stderr, "Note: could not resolve vault path:", err) would be more consistent.
Small nit on the new test
The mock restore at the end of the absolute path sub-test won't run if t.Fatal fires inside the mock. Worth swapping that manual restore for a defer or t.Cleanup.
fix(tests): correct vault names in WSL path conversion tests
* doc: how to install from AUR (Yakitrak#87) * feat: filesystem-first create command and default open-type config (Yakitrak#90) * fix: address PR review feedback * test: rename to the original folder name --------- Co-authored-by: Kartikay Jainwal <kartikayjainwal@gmail.com>
- vault_path.go: return absolute path names directly without reading obsidian.json, handling the common case of storing full paths as the default vault - set_default.go: path resolution after setting the vault name is now best-effort; use fmt.Fprintln(os.Stderr) instead of log.Printf for consistency with surrounding fmt.Println calls - vault_path_test.go: use t.Cleanup for mock restore so it runs even if t.Fatal fires; fix WSL C: drive test to use "Obsidian Vault" as vault name matching the path suffix, per reviewer suggestion
fix: set-default no longer fails when obsidian.json is missing or unparseable
Description
Three fixes:
vault_path.go: if vault name is an absolute path, return it directly without reading obsidian.json (handles the common case of storing full paths as the default vault)
set_default.go: path resolution after setting the name is now best-effort; a warning is printed instead of a fatal error, so the command always succeeds as long as writing preferences.json works
fix tests: correct vault names in WSL path conversion tests. getPathForVault matches by strings.HasSuffix(path, name), so vault names must be a suffix of the configured path. The original tests used arbitrary keys ('windows', 'secondary', 'linux') that never matched, causing all three WSL tests to fail since PR Add Support for WSL Environments With Windows-stored Obsidian Vaults #84.
Motivation and Context
fix some bugs
Checklist: