fix: system.json not found

[ridhoperdana]

Without this we need to copy systems.json to retro folder.

[ridhoperdana]

PR Review: fix: system.json not found

Summary

This PR addresses a common first-run issue where systems.json is missing from the user's config directory. The approach includes packaging the file and auto-copying it, plus an important bug fix for directory existence checking.

---

✅ What's Good

1. Bug fix in search() (lines 183-186): This is a solid fix. The original code would raise FileNotFoundError when system_dir doesn't exist, since os.listdir() was evaluated before the os.path.exists() check. Your fix properly guards the os.listdir() call.

2. MANIFEST.in addition: Including systems.json in the package manifest is the right approach for distributing default config files.

3. .gitignore addition: Standard practice for Python projects.

---

⚠️ Concerns & Recommendations

1. Path Resolution Fragility (Critical)

source = os.path.normpath(os.path.join(pkg_dir, .., systems.json))

Issue: This assumes systems.json lives outside the package directory (one level up). This works in development but will fail after pip install because:

• Installed packages go to site-packages/retro/
• The ../systems.json resolves to site-packages/systems.json, which won't exist

Recommendation: Use importlib.resources (Python 3.7+) or bundle systems.json inside the retro package:

# Option A: Python 3.9+
from importlib.resources import files
source = files(retro).joinpath(systems.json)

# Option B: Older Python
import pkgutil
source = os.path.join(pkg_dir, systems.json)  # Keep it inside the package

Then move systems.json to retro/systems.json and update MANIFEST.in accordingly.

---

2. No Error Handling

if os.path.exists(source):
    shutil.copy(source, self.cfg)

Issue: Silent failures. If shutil.copy fails (permissions, disk full, etc.), the user gets no feedback.

Recommendation:

if os.path.exists(source):
    try:
        shutil.copy(source, self.cfg)
    except (IOError, OSError) as e:
        print(f"Warning: Could not copy systems.json: {e}")

---

3. Race Condition (Minor)

Between os.path.exists(self.cfg) and shutil.copy(), another process could create the file. Consider using shutil.copy2() with existence check inside a try/except.

---

4. Missing Newline in .gitignore

- +venv/ \ No newline at end of file

It's a minor nit, but files should end with a newline. Add an empty line at the end.

---

Additional Suggestion

Consider adding logging instead of silent operations:

import logging
logging.basicConfig(level=logging.INFO)
logger = logging.getLogger(__name__)

def _ensure_systems_json(self):
    if os.path.exists(self.cfg):
        return
    logger.info(f"systems.json not found, creating default at {self.cfg}")
    # ... rest of implementation

---

Verdict

Approve with minor changes requested. The bug fix is correct and the intent is solid, but the path resolution needs to be more robust for installed packages. The current implementation works only in development environments.

Would you like me to submit a patch for the importlib.resources approach?