-
Notifications
You must be signed in to change notification settings - Fork 152
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
feat: add xdg support #826
Conversation
I compiled Polar with the feature of PR and started using it for local development of LN applications. While using Polar compiled from the master branch, I experienced some errors when creating multiple channels. At first, I thought this was caused by my patch but it is not. The most up-to-date versions (take the last two or four commits) of the official master branch have the same errors: I compiled them and ran the App Image to test. So, I created a path of this PR ( |
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.
Thanks for the PR. This is a nice update, especially since it's backwards compatible.
I just requested some minor fixes then this should be good to go.
src/shared/utils.ts
Outdated
|
||
/** | ||
* setup logging to store log files in ~/.polar/logs/ dir | ||
*/ | ||
export const initLogger = () => { | ||
log.transports.file.resolvePath = (variables: log.PathVariables) => { | ||
const ap = app || remote.app; | ||
return join(ap.getPath('home'), '.polar', 'logs', variables.fileName as string); | ||
const home = ap.getPath('home'); | ||
const xdgPath = join(home, '.local/share/polar'); |
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.
I haven't tested but I believe this will fail on Windows because it uses the \
path separator.
Use join(home, '.local', 'share', 'polar')
to make it compatible with all platforms.
src/utils/config.ts
Outdated
/** | ||
* XDG-compliant path where application data is stored | ||
*/ | ||
export const xdgDataPath = join(remote.app.getPath('home'), '.local/share/polar'); |
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.
Same as above
src/utils/config.ts
Outdated
export const dataPath = join(remote.app.getPath('home'), '.polar'); | ||
export const dataPath = existsSync(xdgDataPath) | ||
? xdgDataPath | ||
: join(remote.app.getPath('home'), 'polar'); |
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.
Shouldn't this be .polar
instead of polar
?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #826 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 139 139
Lines 4356 4359 +3
Branches 823 826 +3
=========================================
+ Hits 4356 4359 +3 ☔ View full report in Codecov by Sentry. |
@jamaljsr I have addressed the requested changes. |
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.
tACK LGTM 🚀
Thanks for the fixes.
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.
Ah, I just realized the test coverage dropped. Wouldn't you mind adding a test to cover the missing line?
I'll do it later on. |
If ~/.local/share/polar exists, use it to store application data instead of ~/.polar, abiding to the XDG specification. The xdg directory is not created automatically (the user must have created it manually beforehand) and if it does not exist, the data dir falls back to ~/.polar.
@jamaljsr, tests added. |
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.
Thanks for adding the tests. This looks good to go 🚀
Description
If and only if
~/.local/share/polar
exists, Polar will use it to store application data instead of~/.polar
, abiding to the XDG specification (see https://wiki.archlinux.org/title/XDG_user_directories).This is a backwards compatible update: users who have their data at
~/.polar
will NOT be affected. The~/.local/share/polar
XDG-compliant directory is not created automatically and the user must create it manually to enable the new feature.Note
I tried to import
dataPath
fromsrc/utils/config
insrc/shared/utils
to avoid code duplication but it wouldn't compile and would complaincannot resolve module 'utils/config'
. I tried to use a relative import,import { dataPath } from '../utils/config'
, but the compiler would throw similar errors (it would not resolveshared/<filename>
imports inside../utils/config
). Therefore, there is some duplicated code.