Skip to content
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

Merged
merged 2 commits into from
Jan 9, 2024
Merged

feat: add xdg support #826

merged 2 commits into from
Jan 9, 2024

Conversation

uwla
Copy link
Contributor

@uwla uwla commented Dec 31, 2023

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 from src/utils/config in src/shared/utils to avoid code duplication but it wouldn't compile and would complain cannot 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 resolve shared/<filename> imports inside ../utils/config). Therefore, there is some duplicated code.

@uwla
Copy link
Contributor Author

uwla commented Jan 2, 2024

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 (git diff HEAD~1 > patch), switched to the latest stable version (git checkout v2.0) and built Polar (yarn package). It has been working perfectly so far. Therefore, I am confident my PR does not introduce any bug and the new functionality won't affect users unless they explicitly create the ~/.local/share/polar/ directory.

Copy link
Owner

@jamaljsr jamaljsr left a 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.


/**
* 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');
Copy link
Owner

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.

/**
* XDG-compliant path where application data is stored
*/
export const xdgDataPath = join(remote.app.getPath('home'), '.local/share/polar');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

export const dataPath = join(remote.app.getPath('home'), '.polar');
export const dataPath = existsSync(xdgDataPath)
? xdgDataPath
: join(remote.app.getPath('home'), 'polar');
Copy link
Owner

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-commenter
Copy link

codecov-commenter commented Jan 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0e8d29b) 100.00% compared to head (dfa9729) 100.00%.
Report is 5 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

@uwla uwla force-pushed the feat-xdg-support branch from ee4367e to 00b0ecc Compare January 6, 2024 22:04
@uwla
Copy link
Contributor Author

uwla commented Jan 6, 2024

@jamaljsr I have addressed the requested changes.

Copy link
Owner

@jamaljsr jamaljsr left a 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.

Copy link
Owner

@jamaljsr jamaljsr left a 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?

@uwla
Copy link
Contributor Author

uwla commented Jan 6, 2024

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.

uwla added 2 commits January 8, 2024 12:08
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.
@uwla uwla force-pushed the feat-xdg-support branch from 00b0ecc to dfa9729 Compare January 8, 2024 15:12
@uwla
Copy link
Contributor Author

uwla commented Jan 8, 2024

@jamaljsr, tests added.

Copy link
Owner

@jamaljsr jamaljsr left a 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 🚀

@jamaljsr jamaljsr merged commit cf6e8dd into jamaljsr:master Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants