Skip to content

Conversation

@yaugenst-flex
Copy link
Collaborator

@yaugenst-flex yaugenst-flex commented Oct 27, 2025

Legacy migrations now materialize a fully annotated config.toml through the config manager instead of copying the flat file. Saved configs drop redundant section header comments while keeping inline field notes. Added coverage ensures the migration helper and CLI path output the structured TOML and preserved spacing.

Greptile Overview

Updated On: 2025-10-27 08:45:28 UTC

Greptile Summary

Changes the legacy migration process to output a fully annotated, structured config.toml file instead of copying the flat legacy config file. This produces a TOML file with all default sections and field-level comments, improving documentation and user experience. Also fixes config source override order so runtime changes now correctly take precedence over environment variables.

Major changes:

  • Added finalize_legacy_migration() function that loads legacy config data, creates a ConfigManager, updates sections with legacy values, and saves to structured TOML format with all defaults
  • Modified migrate_legacy_config() to call finalize_legacy_migration() after copying the directory tree
  • Changed serializer to add blank lines between sections instead of section header comments for cleaner output
  • Fixed override precedence in ConfigManager._reload() so runtime overrides apply after environment variables
  • Updated tests to verify the new structured output format and correct override behavior
  • Improved CLI messaging for better user guidance
  • Added deprecation warnings for legacy Env API usage

Confidence Score: 5/5

  • This PR is safe to merge with high confidence
  • The changes are well-tested with comprehensive test coverage for both the helper function and CLI path. Error handling is robust with proper cleanup on failure (removes partially written config.toml if save fails). The override precedence fix is validated by split tests covering both scenarios. All changes are backwards compatible and improve user experience.
  • No files require special attention - all changes are straightforward with good test coverage

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/config/legacy.py 5/5 Added finalize_legacy_migration function to promote legacy flat config files into structured TOML format with proper defaults and error handling. Also added deprecation warnings for Env usage.
tidy3d/config/loader.py 5/5 Added call to finalize_legacy_migration during migration to ensure legacy config is promoted to structured format after directory copy.
tidy3d/config/serializer.py 5/5 Changed behavior to add blank lines between sections in TOML documents instead of section header comments for cleaner output.
tidy3d/config/manager.py 5/5 Fixed config source override order so runtime overrides now correctly take precedence over environment variables.
tidy3d/web/cli/app.py 5/5 Improved CLI message to provide clearer instructions about removing legacy directory after migration.

Sequence Diagram

sequenceDiagram
    participant CLI as CLI (tidy3d config migrate)
    participant Loader as migrate_legacy_config
    participant Copytree as shutil.copytree
    participant Finalize as finalize_legacy_migration
    participant Legacy as load_legacy_flat_config
    participant Manager as ConfigManager
    participant Serializer as serializer.build_document

    CLI->>Loader: migrate_legacy_config()
    Loader->>Copytree: Copy ~/.tidy3d to canonical dir
    Copytree-->>Loader: Files copied
    Loader->>Finalize: finalize_legacy_migration(canonical_dir)
    Finalize->>Legacy: load_legacy_flat_config(config_dir)
    Legacy-->>Finalize: legacy config data dict
    Finalize->>Manager: ConfigManager(config_dir)
    Finalize->>Manager: update_section with legacy values
    Finalize->>Manager: save(include_defaults=True)
    Manager->>Serializer: build_document with defaults
    Serializer-->>Manager: Annotated TOML with blank line separators
    Manager-->>Finalize: config.toml saved
    Finalize->>Finalize: Delete legacy flat config file
    Finalize-->>Loader: Migration finalized
    Loader-->>CLI: canonical_dir path
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

5 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@github-actions
Copy link
Contributor

github-actions bot commented Oct 27, 2025

Diff Coverage

Diff: origin/develop...HEAD, staged and unstaged changes

  • tidy3d/config/legacy.py (70.0%): Missing lines 414-420,426-427
  • tidy3d/config/loader.py (100%)
  • tidy3d/config/manager.py (100%)
  • tidy3d/config/serializer.py (100%)
  • tidy3d/web/cli/app.py (0.0%): Missing lines 200

Summary

  • Total: 36 lines
  • Missing: 10 lines
  • Coverage: 72%

tidy3d/config/legacy.py

Lines 410-424

  410         if isinstance(values, dict):
  411             manager.update_section(section, **values)
  412     try:
  413         manager.save(include_defaults=True)
! 414     except Exception:
! 415         if config_path.exists():
! 416             try:
! 417                 config_path.unlink()
! 418             except Exception:
! 419                 pass
! 420         raise
  421 
  422     legacy_flat_path = config_dir / "config"
  423     if legacy_flat_path.exists():
  424         try:

Lines 422-428

  422     legacy_flat_path = config_dir / "config"
  423     if legacy_flat_path.exists():
  424         try:
  425             legacy_flat_path.unlink()
! 426         except Exception as exc:
! 427             log.warning(f"Failed to remove legacy configuration file '{legacy_flat_path}': {exc}")

tidy3d/web/cli/app.py

Lines 196-204

  196     click.echo(f"Configuration migrated to '{destination}'.")
  197     if delete_legacy:
  198         click.echo("The legacy '~/.tidy3d' directory was removed.")
  199     else:
! 200         click.echo(
  201             f"The legacy directory remains at '{legacy_dir}'. "
  202             "Remove it after confirming the new configuration works, or rerun with '--delete-legacy'."
  203         )

@yaugenst-flex yaugenst-flex force-pushed the yaugenst-flex/config-fixes branch from 8ed7ca8 to 5687b79 Compare October 27, 2025 08:42
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

8 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@yaugenst-flex yaugenst-flex force-pushed the yaugenst-flex/config-fixes branch from 5687b79 to d50cd4c Compare October 27, 2025 10:38
@yaugenst-flex yaugenst-flex added this pull request to the merge queue Oct 27, 2025
Merged via the queue into develop with commit 4929337 Oct 27, 2025
35 checks passed
@yaugenst-flex yaugenst-flex deleted the yaugenst-flex/config-fixes branch October 27, 2025 12:29
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