Skip to content

Conversation

cvanelteren
Copy link
Collaborator

@cvanelteren cvanelteren commented Oct 8, 2025

Closes #364

This pull request resolves a circular import error that occurred when setting the cycle parameter from a user rc file. It also includes a major architectural refactoring of the configuration system to make it more robust, extensible, and easier to maintain.

The Problem

The initial issue was caused by a circular dependency:

  • ultraplot.config needed to import from ultraplot.colors to validate and process the cycle setting.
  • ultraplot.colors needed to import the rc object from ultraplot.config for its own internal configuration.

This circular dependency made the application's initialization fragile and order-dependent, leading to the reported ImportError.

The solution: adding handlers to Configurator

Instead of a temporary patch, this PR implements a more fundamental solution by refactoring the configuration system to follow the Dependency Inversion Principle.

The key changes are:

  1. Decoupled config and colors modules: The Configurator in config.py is now completely agnostic of color-specific logic. It no longer imports from colors.py.
  2. Introduced a Handler Registry: The Configurator now has a mechanism (register_handler) that allows other modules to register their own functions for handling specific settings.
  3. Moved Logic to the Responsible Module: The logic for processing the cycle setting has been moved into a _cycle_handler function within colors.py. This module is now responsible for registering its handler with the Configurator.
  4. Removed Workarounds: This architectural change completely eliminates the circular dependency, allowing for the removal of all previous workarounds, including the skip_cycle parameter and the complex deferred settings logic.

Added tests

This refactoring was accompanied by a significant testing effort to ensure its correctness and prevent regressions:

  1. New Handler Test: A new test file, test_handlers.py, was created to specifically unit-test the new handler registration and execution mechanism.
  2. Hardened Configuration Test: The test for loading rc files (test_config.py) was completely rewritten to be more robust. After a long debugging process, the final version now directly and atomically tests the rc.load() method, ensuring the test is isolated from the user's local environment and any module-reloading side effects.

PR fixes:

  • The circular dependency is resolved.
  • The initialization process is now more robust, predictable, and extensible.
  • The changes are thoroughly tested with both new and improved tests.

Copy link

codecov bot commented Oct 8, 2025

Codecov Report

❌ Patch coverage is 95.34884% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
ultraplot/tests/test_handlers.py 86.66% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@cvanelteren cvanelteren requested a review from Copilot October 8, 2025 16:35
Copilot

This comment was marked as resolved.

@cvanelteren cvanelteren marked this pull request as draft October 8, 2025 16:47
@cvanelteren
Copy link
Collaborator Author

@beckermr I am pushing a more profound change that is more robust and less frail as the one pushed now. I will update the text above. One moment.

@cvanelteren cvanelteren force-pushed the fix-circular-import-rc-setting branch from 50b913a to 79b45de Compare October 8, 2025 17:03
@cvanelteren
Copy link
Collaborator Author

Note that the handlers are only intended for settings that are "problematic" like the one with cycle. I think this PR is cleaner now as we separate the dependency between the configurator rand colors. The Configurator now does not need to know how colors handles the property.

@cvanelteren cvanelteren marked this pull request as ready for review October 8, 2025 17:13
@cvanelteren
Copy link
Collaborator Author

Fingers crossed that the test pass 🤞

@beckermr beckermr enabled auto-merge (squash) October 8, 2025 17:22
@beckermr beckermr disabled auto-merge October 8, 2025 17:28
@cvanelteren
Copy link
Collaborator Author

Will wait for the tests to pass and then merge this. Just added the API docs for this -- in case we forget in the future (likely ;-)).

@cvanelteren cvanelteren enabled auto-merge (squash) October 8, 2025 17:33
@cvanelteren cvanelteren disabled auto-merge October 8, 2025 17:33
@cvanelteren
Copy link
Collaborator Author

cvanelteren commented Oct 8, 2025

  • check produced docs on docstring
image

@cvanelteren cvanelteren enabled auto-merge (squash) October 8, 2025 17:46
@cvanelteren cvanelteren merged commit 0fc1873 into Ultraplot:main Oct 8, 2025
25 checks passed
@cvanelteren cvanelteren deleted the fix-circular-import-rc-setting branch October 8, 2025 18:00
@cvanelteren cvanelteren restored the fix-circular-import-rc-setting branch October 9, 2025 04:02
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.

circular import when loading rc with cycle

2 participants