Conversation
os.makedirs(..., exist_ok=True) raises FileExistsError when a path component is a symlink whose target does not exist. get_config() therefore crashed on configurations that point rsr_dir or rayleigh_dir behind such a symlink. Create the link target instead. Closes pytroll#266
mraspaud
left a comment
mraspaud
left a comment
There was a problem hiding this comment.
Thanks a lot for looking into this. Comment inline.
Manny7717
left a comment
Manny7717
left a comment
There was a problem hiding this comment.
Thanks for the suggestion — you're right, os.path.realpath resolves dangling symlinks to their (nonexistent) target, so the try/except was unnecessary. Simplified _ensure_dir to a single os.makedirs(os.path.realpath(path), exist_ok=True) call in 5d9e17d. All 3 tests in test_config.py still pass and flake8 is clean.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #293 +/- ##
==========================================
+ Coverage 91.17% 91.24% +0.07%
==========================================
Files 25 26 +1
Lines 2673 2697 +24
==========================================
+ Hits 2437 2461 +24
Misses 236 236
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
djhoese
left a comment
djhoese
left a comment
There was a problem hiding this comment.
Thanks. I just had a couple suggestions.
| cfg_file = tmp_path / "cfg.yaml" | ||
| cfg_file.write_text("".join(f"{key}: {value}\n" for key, value in settings.items())) | ||
|
|
||
| get_config(str(cfg_file)) |
There was a problem hiding this comment.
get_config should accept a Path so this conversion seems unnecessary.
| get_config(str(cfg_file)) | |
| get_config(cfg_file) |
| cfg_file = tmp_path / "cfg.yaml" | ||
| cfg_file.write_text(f"rsr_dir: {data}\nrayleigh_dir: {data}\n") | ||
|
|
||
| get_config(str(cfg_file)) |
There was a problem hiding this comment.
Same here.
| get_config(str(cfg_file)) | |
| get_config(cfg_file) |
| assert other.is_dir() | ||
|
|
||
|
|
||
| def test_get_config_creates_missing_directories(tmp_path): |
There was a problem hiding this comment.
Could this test be combined with the above test? They seem like similar situations so parametrize could be used to select either a missing directory (this test) or a symlink (the above test).
|
Many thanks for this! I have no further suggestions. Would be good to have it merged I think. Do you have time addressing the few comments above related to the unit tests @Manny7717 ? |
4 participants
What
Fixes #266:
get_config()crashed withFileExistsErrorwhenrsr_dirorrayleigh_diris configured behind a symlink whose target does not exist.os.makedirs(..., exist_ok=True)raisesFileExistsErrorwhen a path component is a symlink whose target is missing — reproduced locally:Change
config.pynow creates the symlink target in that case, so directories behind symlinks behave like ordinary directories. Verified to fail without the fix and pass with it.Tests
test_get_config_tolerates_dangling_symlink(parametrized overrsr_dir/rayleigh_dir):get_configsucceeds and the link target is created.test_get_config_creates_missing_directories: ordinary missing directories still work.