feat: respect $XDG_CONFIG_HOME for config directory#26
Merged
Conversation
Default config location is now $XDG_CONFIG_HOME/devskim (falling back to ~/.config/devskim per XDG spec). Existing installs with ~/.devskim are detected automatically and continue to work unchanged.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSwitches runtime config resolution to $XDG_CONFIG_HOME/devskim (default ~/.config/devskim), adds resolvers for config/cache/data with legacy ~/.devskim fallback for config, moves seen.json to DATA_DIR, updates tests, and revises README and Config docstring. ChangesXDG Config Directory Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #26 +/- ##
==========================================
+ Coverage 62.44% 70.99% +8.54%
==========================================
Files 18 18
Lines 988 1010 +22
==========================================
+ Hits 617 717 +100
+ Misses 371 293 -78 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_config.py (1)
9-44: ⚡ Quick winAdd one resolver test for relative
XDG_CONFIG_HOME.Current branch coverage is good, but an explicit relative-path case will prevent regressions around fallback behavior.
Suggested test
+def test_resolve_config_dir_relative_xdg_env_falls_back_to_default(tmp_path): + env = dict(os.environ) + env["XDG_CONFIG_HOME"] = "relative/path" + with patch.dict(os.environ, env, clear=True): + with patch("devskim.config.Path") as mock_path: + mock_path.home.return_value = tmp_path + result = _resolve_config_dir() + assert result == tmp_path / ".config" / "devskim"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_config.py` around lines 9 - 44, Add a new unit test (e.g., test_resolve_config_dir_xdg_relative) that covers a relative XDG_CONFIG_HOME value: create a directory under tmp_path (xdg), create tmp_path/xdg/devskim, set os.environ["XDG_CONFIG_HOME"] to a relative path string (not absolute) and ensure the code resolves it correctly by either chdiring to an appropriate parent directory or using os.path.relpath to produce the relative string; call _resolve_config_dir() and assert the result equals the resolved absolute path (xdg/devskim). This mirrors the existing tests (test_resolve_config_dir_xdg_env, test_resolve_config_dir_xdg_default) and ensures _resolve_config_dir handles relative XDG_CONFIG_HOME values without falling back to legacy paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devskim/config.py`:
- Around line 27-29: The CACHE_PATH is incorrectly placed under CONFIG_DIR;
create a separate XDG-aware cache directory resolver (e.g.,
_resolve_cache_dir()) that reads $XDG_CACHE_HOME and falls back to
~/.cache/devskim, use that to set CACHE_DIR and then CACHE_PATH = CACHE_DIR /
"cache.json", and update the other cache-related assignments (the same pattern
around the other CACHE_PATH/CACHE_DIR usage) to use this new resolver so cache
files live under the XDG cache location instead of CONFIG_DIR; ensure the
resolver returns a pathlib.Path and that calling code creates the directory if
missing.
- Around line 16-18: Check that the XDG_CONFIG_HOME environment value (variable
xdg) is an absolute path before using it; if not absolute, ignore it and fall
back to the existing default logic instead of returning Path(xdg) / "devskim".
Update the code around xdg and the Path(xdg) / "devskim" return to validate
absoluteness (e.g., os.path.isabs or Path(xdg).is_absolute) and only use the XDG
path when that check passes.
In `@README.md`:
- Around line 60-63: Replace hardcoded `~/.config/devskim/config.toml` examples
with a reference that uses the XDG variable and its fallback (e.g.
`${XDG_CONFIG_HOME:-~/.config}/devskim/config.toml`) wherever the README shows
the edit path (including the other occurrence noted), and update the stale
feature bullet that still mentions `~/.devskim/config.toml` to describe the
XDG-compliant location and fallback behavior instead.
---
Nitpick comments:
In `@tests/test_config.py`:
- Around line 9-44: Add a new unit test (e.g.,
test_resolve_config_dir_xdg_relative) that covers a relative XDG_CONFIG_HOME
value: create a directory under tmp_path (xdg), create tmp_path/xdg/devskim, set
os.environ["XDG_CONFIG_HOME"] to a relative path string (not absolute) and
ensure the code resolves it correctly by either chdiring to an appropriate
parent directory or using os.path.relpath to produce the relative string; call
_resolve_config_dir() and assert the result equals the resolved absolute path
(xdg/devskim). This mirrors the existing tests (test_resolve_config_dir_xdg_env,
test_resolve_config_dir_xdg_default) and ensures _resolve_config_dir handles
relative XDG_CONFIG_HOME values without falling back to legacy paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 953fbbcc-b799-4d60-82b9-036313cba563
📒 Files selected for processing (3)
README.mddevskim/config.pytests/test_config.py
Cache files now live under $XDG_CACHE_HOME/devskim (default ~/.cache/devskim) instead of the config directory. Adds _resolve_cache_dir() mirroring the existing config resolver pattern.
Persistent read-state belongs under XDG_DATA_HOME (~/.local/share/devskim) not the config directory. Adds _resolve_data_dir() and DATA_DIR constant, updates seen.py to use DATA_DIR.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devskim/config.py`:
- Around line 22-23: The fallback currently returns the path represented by
legacy even when it is a regular file; update the condition in the selection
logic that uses the symbols legacy and xdg_default so it only accepts legacy
when it exists and is a directory (use legacy.is_dir()) and xdg_default does not
exist; i.e., add the directory check to prevent returning a file path as the
fallback target.
In `@tests/test_config.py`:
- Around line 22-26: The test currently checks for a non-prefix "relative"
string which can miss other relative forms; change the assertion in
test_resolve_config_dir_xdg_relative_ignored (and the other relative-XDG test at
the other location) to assert the path is absolute instead (e.g., use
Path(result).is_absolute() or os.path.isabs(result)) so the guard strictly
verifies absoluteness of _resolve_config_dir's return value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 06390989-7ab1-42de-a985-03346230db3a
📒 Files selected for processing (2)
devskim/config.pytests/test_config.py
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/test_config.py (1)
23-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStrengthen relative-path guards with absolute-path assertions
At Line 26, Line 153, and Line 174, string-prefix checks are weaker than asserting absoluteness directly. Use
result.is_absolute()to make these tests robust against alternate relative forms.Suggested fix
def test_resolve_config_dir_xdg_relative_ignored(): with patch.dict(os.environ, {"XDG_CONFIG_HOME": "relative/path"}, clear=False): result = _resolve_config_dir() - assert not str(result).startswith("relative") + assert result.is_absolute() @@ def test_resolve_cache_dir_xdg_relative_ignored(): with patch.dict(os.environ, {"XDG_CACHE_HOME": "relative/cache"}, clear=False): result = _resolve_cache_dir() - assert not str(result).startswith("relative") + assert result.is_absolute() @@ def test_resolve_data_dir_xdg_relative_ignored(): with patch.dict(os.environ, {"XDG_DATA_HOME": "relative/data"}, clear=False): result = _resolve_data_dir() - assert not str(result).startswith("relative") + assert result.is_absolute()Also applies to: 150-154, 171-175
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_config.py` around lines 23 - 27, The test test_resolve_config_dir_xdg_relative_ignored currently checks that the resolved path doesn't start with a literal "relative" string; change the assertion to check path absoluteness by calling _resolve_config_dir() and asserting result.is_absolute() (use pathlib.Path.is_absolute()) so the guard rejects any relative input forms; update the analogous assertions in the other tests referenced (around lines with similar prefix checks) to use .is_absolute() on the _resolve_config_dir() return value instead of string-prefix checks.devskim/config.py (1)
22-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard legacy fallback with a directory check
Line 22 should require
~/.devskimto be a directory, not just an existing path; otherwise a file at that location can cause runtime failure when creating/writing config files.Suggested fix
- if legacy.exists() and not xdg_default.exists(): + if legacy.is_dir() and not xdg_default.exists(): return legacy🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@devskim/config.py` around lines 22 - 23, The fallback check currently allows any existing path at `legacy` (e.g., ~/.devskim) including files; change the condition to require a directory by using `legacy.is_dir()` (or combining `legacy.exists() and legacy.is_dir()`) instead of `legacy.exists()` so the fallback only returns `legacy` when it is a directory and `xdg_default` does not exist; update the conditional around the `legacy` and `xdg_default` variables accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@devskim/config.py`:
- Around line 22-23: The fallback check currently allows any existing path at
`legacy` (e.g., ~/.devskim) including files; change the condition to require a
directory by using `legacy.is_dir()` (or combining `legacy.exists() and
legacy.is_dir()`) instead of `legacy.exists()` so the fallback only returns
`legacy` when it is a directory and `xdg_default` does not exist; update the
conditional around the `legacy` and `xdg_default` variables accordingly.
In `@tests/test_config.py`:
- Around line 23-27: The test test_resolve_config_dir_xdg_relative_ignored
currently checks that the resolved path doesn't start with a literal "relative"
string; change the assertion to check path absoluteness by calling
_resolve_config_dir() and asserting result.is_absolute() (use
pathlib.Path.is_absolute()) so the guard rejects any relative input forms;
update the analogous assertions in the other tests referenced (around lines with
similar prefix checks) to use .is_absolute() on the _resolve_config_dir() return
value instead of string-prefix checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e86b47b-85c2-4878-ba13-ef7b84210b0b
📒 Files selected for processing (4)
devskim/config.pydevskim/seen.pytests/test_config.pytests/test_seen.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Config and cache files now follow the XDG Base Directory spec.
Behaviour
$XDG_CONFIG_HOMEset$XDG_CONFIG_HOME/devskim/$XDG_CONFIG_HOMEunset~/.config/devskim/~/.devskimexists and XDG path does not~/.devskim/(legacy, no migration needed)Existing installs are detected automatically — no manual migration required.
Changes
config.py:_resolve_config_dir()replaces the hardcodedPath.home() / ".devskim"README.md: updated config path referencestests/test_config.py: four new tests covering all resolution branchesSummary by CodeRabbit
Documentation
New Features
Tests