-
Notifications
You must be signed in to change notification settings - Fork 43
Use platformdirs package for all user-related file paths #3166
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
Shouldn't this be now against |
Yes. I'll rebase later today |
…ed from the old location to the new
…ig file not working)
…t-creation to bypass initial permissions issue
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.
It looks like I was wrong at the fortnightly meeting. The problem isn't a GitHub action permission error like I expected as I'm also able to reproduce it on my machine.
I have left a comment on the line I think is the culprit.
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.
It now works on my machine, and code LGTM.
I've just noticed this is still marked as a draft. Is there any work left do on this? |
Two items left:
|
… functionality in other parts of the code
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.
Gates Failed
Enforce advisory code health rules
(1 file with Complex Method)
Gates Passed
3 Quality Gates Passed
See analysis details in CodeScene
Reason for failure
Enforce advisory code health rules | Violations | Code Health Impact | |
---|---|---|---|
user.py | 1 advisory rule | 10.00 → 9.69 | Suppress |
Quality Gate Profile: Clean Code Collective
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
I updated the PR description.
Thanks, I updated the documentation with this.
The log file gets copied to the new directory on app launch, if no log exists in the new location.
Fixed.
Beyond the PR scope at this point.
Fixed.
Another beyond the PR scope, though, noted. |
…e included in future versions of sasview
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.
Code LGTM. Just a few comments regarding the documentation.
…tion to clarify the difference
Description
This moves our user file handling to the
platformdirs
package for os preferred pathing. This is used to store log files, configurations, and app data. This was suggested by @llimeht in the wheels discussion in #3161. This automatically creates subfolders for each version so docs no longer require versioning in their paths.What has changed:
~/.sasview
Good ideas, but outside the scope of this PR
Other work to be done:
Fixes #3142
How Has This Been Tested?
CI is able to launch both the Windows and Ubuntu 22 binaries.
Review Checklist:
Documentation (check at least one)
Installers
Licencing (untick if necessary)