-
Notifications
You must be signed in to change notification settings - Fork 826
refactor: simplify home.Dir()
#1353
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| return tmp | ||
| }) | ||
| // Dir returns the user home directory. | ||
| func Dir() string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could probably keep the OnceValue though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Otherwise, this is a solid one, @andreynering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@caarlos0 I want to remove OnceValue because it hold a sync.Mutex inside and it locks reads.
If we want to cache it, then I think we can just compute on func init() and hold on a global unexported variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made changes. Take a look again.
32c130f to
74f63ac
Compare
meowgorithm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done. Might be worth at least logging the error but up to you.
Fwiw, here's the os.UserHomeDir definition.
https://cs.opensource.google/go/go/+/refs/tags/go1.25.3:src/os/file.go;l=608-627
* Realistically, this should almost never fail. * If it does, returning `""` probably makes more sense than a temp dir. Empty means Go will assume the working directory. * Getting rid of `sync.Once` is good as it locks and this can be called on every render cycle. (Used to compute `~` on the sidebar, etc).
74f63ac to
5b72be5
Compare
""probably makes more sense than a temp dir. Empty means Go will assume the working directory.sync.Onceis good as it locks and this can be called on every render cycle. (Used to compute~on the sidebar, etc).