Respect local environment configuration for app file storage - #4606
Respect local environment configuration for app file storage#4606100jinwoo001 wants to merge 17 commits into
Conversation
01ba65b to
2e8bbfb
Compare
…#2867) Signed-off-by: 100jinwoo001 <rahul.patel09@infosys.com>
2e8bbfb to
f26bc36
Compare
|
This PR is marked draft, but it's passing CI - was it your intention to have this reviewed? As an aside - there's no need to include "testing" details when the details are "I ran the test suite". A passing test suite is not negotiable, so it's not notable. |
shinzoxD
left a comment
shinzoxD
left a comment
There was a problem hiding this comment.
Thanks for carrying this across GTK, Qt, Textual, and WinForms. I found two correctness/coverage gaps that need addressing before this satisfies Toga's contribution rules:
-
The XDG variables are accepted without checking that they are absolute. The XDG Base Directory specification requires relative values to be treated as invalid and ignored (https://specifications.freedesktop.org/basedir/latest/#basics). With XDG_CONFIG_HOME=relative-config, for example, this head returns relative-config/; toga.Paths.config then creates that directory relative to the process working directory, despite Toga documenting app paths as absolute. Please validate all four XDG values with Path.is_absolute() and fall back to their defaults when unset, empty, or relative; include the invalid-relative case in the tests.
-
The AppProbe changes adapt expected values to the same environment, but CI does not set non-default XDG_*_HOME or LOCALAPPDATA values. On the current runners these assertions still exercise only the old default paths, so the testbed would pass even if the production methods continued ignoring the variables. Please add a regression path that sets non-default absolute values before app.paths is cached and verifies config/data/cache/logs on Linux and Windows.
Because this is a user-visible behavioral change, the repository constitution also requires updating the Paths documentation (docs/en/reference/api/data-representation/paths.md or the file-management topic) to describe these environment overrides. The change note itself is present and clear.
3e83636 to
0f23bd8
Compare
…d CI regression tests
0f23bd8 to
2f71930
Compare
|
Closing as there's been no response from the PR author to a direct question in over a week. @shinzoxD Please familiarise yourself with our project AI policy. We do not want random drive-by "reviews" from automated agents. |
|
Hi @freakboy3742 , I had provided the PR code for the Black Duck scan and was waiting for the green signal before raising the actual PR, so that’s why there was a delay from my side. |
No worries - for future reference, if you're asked a question, it's generally a good idea to respond, even if the answer is "not yet, I'm waiting on something". I'm also I'm not sure what a "Black Duck" scan is - it's not something from our contribution guide. Your PR is, however, failing pre-commit checks - something our contribution guide does cover. Lastly - please don't force push repositories. When you force push, we lose context of any historical reviews. In this case, there haven't been any reviews yet - but it's a good habit to get into. We don't care about a "messy" commit history, as we collapse commits on merge. |
…p._impl.paths" This reverts commit d28cd44.
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
Thanks for the PR; this mostly looks good, but a couple of simplifications, and some modifications that seem to be unrelated have been flagged inline.
| ) | ||
| def test_cant_reassign(app, path_name): | ||
| """App path attributes are read-only.""" | ||
| # Theoretically, this could leak out of this test... but only if it fails! |
There was a problem hiding this comment.
Please don't remove comments that are still accurate, correct, and unrelated to the changes that you're making.
| self.native.setSizePolicy( | ||
| QSizePolicy.Policy.Expanding, | ||
| QSizePolicy.Policy.Fixed, | ||
| ) |
There was a problem hiding this comment.
What does this have to do with paths?
There was a problem hiding this comment.
Again - what do these changes have to do with paths?
| local_app_data = os.environ.get("LOCALAPPDATA") | ||
| base_dir = ( | ||
| Path(local_app_data) if local_app_data else (Path.home() / "AppData/Local") | ||
| ) |
There was a problem hiding this comment.
Again, os.environ.get() has a fallback mode.
| local_app_data = os.environ.get("LOCALAPPDATA") | ||
| base_dir = ( | ||
| Path(local_app_data) if local_app_data else (Path.home() / "AppData/Local") | ||
| ) |
There was a problem hiding this comment.
Same again - fallback mode can be used to simplify things.
Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>
3 participants
Summary
This PR updates the app file storage logic to respect the local environment configuration instead of always using the default platform-specific paths.
Fixes #2867.
PR Checklist:
Assisted-by: ChatGPT