MacOS Pal Support: Docs and Code Enhancements#31
MacOS Pal Support: Docs and Code Enhancements#31mik-tf wants to merge 2 commits intoscottyeager:mainfrom
Conversation
scottyeager
left a comment
scottyeager
left a comment
There was a problem hiding this comment.
Thanks for the PR! See the comments per file for some needed changes.
There was a problem hiding this comment.
Let's just use curl. It should always be there on Linux too, so we can keep it simple.
| var storageDir string | ||
| if runtime.GOOS == "darwin" { | ||
| storageDir = filepath.Join(homeDir, "Library", "Application Support", "pal_helper") | ||
| } else { |
There was a problem hiding this comment.
Technically on the off chance that someone tries pal on FreeBSD, etc, they will also fall under this branch. As far as I can tell, using the XDG standard on these systems is okay. Maybe we can just mention this possibility in the comment, so it doesn't look like we are using a catchall else to mean linux.
|
|
||
| // Clean up the old file if it exists. This shouldn't slow us down if the | ||
| // file no longer exists, since stat hits cached data | ||
| homeDir, _ := os.UserHomeDir() |
There was a problem hiding this comment.
We should not be silently dropping the possible err here, I think. Better to keep what we had I think:
if err != nil {
return fmt.Errorf("failed to get home directory: %w", err)
| storagePath, err := GetAbbrFilePath() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get home directory: %w", err) | ||
| return err |
There was a problem hiding this comment.
Let's also wrap this error to provide context. Not sure if we are doing that totally consistently, but it seems like a good idea.
| ## Quickstart | ||
|
|
||
| It's a single binary that you can just download into any location on your `$PATH`: | ||
| It's a single binary that you can download into any location on your `$PATH`: |
There was a problem hiding this comment.
We can standardize on curl here too (see comment below). But why do we have a different curl invocation in the update command versus here on the README?
Also, let's put Linux at the top of the list :)
| return `set -l pal_prefix "` + abbreviationPrefix + `"` + "\n" + FishAbbrEmbed + "\n" | ||
| path, err := inout.GetAbbrFilePath() | ||
| if err != nil { | ||
| // Fallback to Linux default if something goes wrong |
There was a problem hiding this comment.
Let's not blindly continue on error. Same for zsh.
|
Update here. I decided against using Changing to curl is probably still relevant. Need to double check the rest. |
|
Great! So I guess we can close this one. |
2 participants
Work Done