-
Notifications
You must be signed in to change notification settings - Fork 942
Change for XDG complicance #1318
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
My python skills are super rusty(I used python about ~4 years ago) so, you can fix it to be more 'pythonic'! |
@lra Updated commit to be more structured, added a test for XDG config (but isn't extensively testing as the code relies on the existence of a file in disk). |
@lra May I kindly ask you to review my PR? I would be very thankful if XDG landed in mackup; It's one of the five apps in my computer that doesn't follow XDG compliance. Is there is anything that is blocking from merging? |
@lra is there any interest in merging this? Does more work need done? I'd be more than happy to help get this feature in. |
Sure! I just need to protected time to get my head around it ;) |
PR duplicates(#632). |
@yamatsum Oops, Why did I not realize that! I should also have changed that... Maybe I'll try to resolve the conflicts, fix that problem and make a new PR tomorrow or the day after that. |
@pcr910303 I think your PR is great!
|
OK, I'm working on this. So I didn't want to have nested directories in Would this be fine? Or... should I just revert to using a directory? 😢 |
@pcr910303 Thank you for a great job! |
""" | ||
xdg_cfg_dir = get_xdg_cfg_dir() | ||
home_custom_apps_dir = os.path.join(os.environ["HOME"], constants.CUSTOM_APPS_DIR) | ||
if os.path.isdir(xdg_cfg_dir): |
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.
If MACKUP_XDG_CONFIG_FILE
does not exist, but xdg_cfg_dir
does exist, is that okay?
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 guess that would force using the default config?
Sorry for the late reply. I'm pretty busy, I think I won't be able to touch this PR for a while (probably until mid-June), but I just updated the PR to change it to |
@lra If you don't have any problems, can you merge? |
I started reviewing it, and was stopped, I can't blindly merge and break everyone's install, it's not as simple as this is a chicken/egg problem, the mackup config is impacting all other configs and must be loaded before anything else, but if it's in a place that depends on a config value, it won't work. If there was a test to check this, it would be easier to review. So I need more time to make sure this does not break, sorry. Also I'd like to respect #632 and see if there is a way to merge both. |
Yeah, this PR is not really ready as @lra said. I'll try to make it ready, but it won't work in it's current state. |
Ok, I'm sorry about abandoning this PR, but I clean-installed macOS Big Sur on my machine and I'm not using Mackup anymore (not that it's not useful, but I lost my Mackup backup so... 🤦🏻♀️), so I don't think I'll have enough time to continue this PR . Maybe someone might continue this, but I'm closing this for now. |
Fixes #1317 :-)