Skip to content

feat(t2-3): replace global config singleton with local TycoonConfig construction - #172

Merged
JesuFemi-O merged 7 commits into
feat/m2-t2-1from
feat/m2-t2-3
Aug 2, 2026
Merged

feat(t2-3): replace global config singleton with local TycoonConfig construction#172
JesuFemi-O merged 7 commits into
feat/m2-t2-1from
feat/m2-t2-3

Conversation

@JesuFemi-O

@JesuFemi-O JesuFemi-O commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Removes the from tycoon.config import config global singleton import from four ingestion command modules: sources.py, run_all.py, sync_cmd.py, and explore.py
  • Each command now constructs TycoonConfig(project_root=_find_project_root()) at invocation time, so every CLI call gets a fresh, correctly-rooted config without shared mutable state
  • sources._require_project() now returns the TycoonConfig it builds so all helpers receive it as an explicit argument; _maybe_auto_scaffold gains a cfg keyword parameter
  • Tests drop monkeypatch.setattr(mod, "config", ...) in favour of monkeypatch.chdir(tmp_path) — simpler and more faithful to real execution

Files to focus on

Production changes (2 patterns, 4 files):

Test changes (drop singleton patching):

Test plan

  • uv run ruff check src tests — clean
  • uv run pytest -q — 690 passed, 3 skipped

Part of milestone M2 (#83). Stacked on feat/m2-t2-2.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

Closes #94

@gemini-code-assist

Copy link
Copy Markdown

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@db-tycoon-stephen

Copy link
Copy Markdown
Contributor

Cleanest PR of the stack. The conversion is complete and consistent across all four command modules, and the test churn is genuinely just cleanup — I checked the three test_explore.py tests that lost their monkeypatch.setattr(..., "config", ...) binding, and all three already had monkeypatch.chdir(tmp_path), so they still assert what they claim rather than passing for the wrong reason. No assertions were dropped anywhere in the -72 lines.

One dead call: cfg.reload() at sources.py:494 is now a no-op. Under the singleton it was load-bearing — it refreshed shared state for anything running later in the same process. Now cfg is function-local and never read again after that line (add_source only calls success(), _maybe_install_*(), and next_steps() before returning), so it just re-reads a file and throws the result away. Safe to drop.

Two structural notes, neither blocking:

TycoonConfig(project_root=_find_project_root()) is now duplicated at four call sites, importing a private helper across module boundaries. sources.py already has the better shape — _require_project() returning the cfg. Promoting that into config.py as something like load_config() or TycoonConfig.from_cwd() would give one public entry point and stop _find_project_root leaking out of the module that owns it.

The singleton is still live in transform.py, db.py, and status.pytest_templates_e2e.py::_rebind_config still has to patch transform_mod.config and cfg_mod.config, which is the tell. Perfectly reasonable to land the migration in slices, but worth an issue tracking the remaining modules so the codebase doesn't settle permanently into two competing patterns.

@db-tycoon-stephen db-tycoon-stephen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes on the items in the comment above: drop the now-dead cfg.reload() at sources.py:494, and promote the duplicated TycoonConfig(project_root=_find_project_root()) into a public factory rather than importing a private helper across four modules.

Also worth opening an issue for the singleton still live in transform.py/db.py/status.py so the two patterns don't become permanent. Strongest PR of the stack otherwise — the test cleanup checks out.

@db-tycoon-stephen db-tycoon-stephen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All three addressed. load_config() is the right shape and the docstring pointing people away from _find_project_root is a nice touch — no imports of it left in commands/. You also caught the second dead reload() in remove_source that I missed. Thanks for filing #175 for the remaining modules.

Writes metadata: defaults and bumps version to 0.2.0 in existing
tycoon.yml files. Operates on raw YAML so comments and ordering are
preserved. Idempotent — second call returns False with no file write.
…ion bump

Instantiate MetadataConfig once rather than twice. Guard the version bump
so a file already at a future schema version is not written back down to
SCHEMA_VERSION. Update docstring to accurately state that comments are not
preserved through the yaml round-trip.
…a_version field

Switch migrate_project from yaml.safe_load/yaml.dump to ruamel.yaml so
comments and blank lines survive the round-trip. Add schema_version as a
separate field on TycoonProject so the user's version field is never
touched by migration. Add ruamel-yaml==0.19.1 as a runtime dependency.

Adds two new tests: comments_preserved and user_version_not_overwritten.
…e versions

Change SCHEMA_VERSION from a semver string to an integer (2) so version
comparisons are unambiguous — string comparison would incorrectly treat
"0.10.0" < "0.2.0" as True. Change schema_version field type to int | None.

Add type guard before comparison: a float schema_version (e.g. 0.2 written
unquoted in YAML) now raises ValueError with a clear message rather than
a TypeError. A schema_version newer than SCHEMA_VERSION also raises rather
than silently passing.

Add two new tests covering both error paths.
…onstruction

Each ingestion command (sources, run_all, sync_cmd, explore) now
constructs TycoonConfig(project_root=_find_project_root()) at invocation
time rather than importing the module-level singleton. Tests drop
monkeypatch.setattr calls in favour of monkeypatch.chdir so _find_project_root
resolves the correct tmp directory without brittle module-level patching.

Closes #94
Add load_config() to config.py as a public entry point that wraps
TycoonConfig(project_root=_find_project_root()), removing the need for
command modules to import the private _find_project_root across module
boundaries. Update all four ingestion command files to use it.

Drop two dead cfg.reload() calls in sources.py (add_source and
remove_source): cfg is function-local and nothing reads from it again
after either call.

Tracks remaining singleton uses in transform.py / db.py / status.py
in issue #175.
@JesuFemi-O
JesuFemi-O merged commit 01a38af into feat/m2-t2-1 Aug 2, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants