Refactors config to accept mapping, removes INI support#8
Conversation
Updates configuration handling to use a Python mapping instead of an INI file, simplifying initialization and validation. Removes all file-based config logic, adjusts documentation and tests accordingly, and improves type safety for clustered Redis config. Enhances maintainability and reduces test setup complexity.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughMigrates FQ from INI file-based configuration to in-memory Python mappings. Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR refactors FQ’s configuration system to accept an in-memory Python mapping (dict-like) instead of reading an INI file, removing file-based config handling and updating tests/docs accordingly.
Changes:
- Replace INI file parsing with
Mapping-based config validation/normalization inFQ. - Update tests to build config dictionaries via a shared
build_test_config()helper and useFQ.close(). - Remove bundled INI config files and update README examples to reflect mapping-based configuration.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/fq/queue.py |
Switches constructor to accept a mapping, removes INI loading/reload, and reads config from dict sections. |
tests/config.py |
Adds shared test config + helper to simplify per-test overrides. |
tests/test_queue.py |
Updates setup/teardown to use mapping config and close(). |
tests/test_func.py |
Removes temp config-file creation and switches to mapping-based config setup. |
tests/test_edge_cases.py |
Adjusts edge-case coverage for mapping config (including clustered boolean validation). |
README.md |
Updates configuration and usage examples to pass a Python mapping. |
tests/test.conf |
Removes INI test config fixture. |
src/fq/default.conf |
Removes default INI config file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fq_config = self.config["fq"] | ||
| redis_config = self.config["redis"] | ||
|
|
||
| self._key_prefix = self._config.get("redis", "key_prefix") | ||
| self._job_expire_interval = int(self._config.get("fq", "job_expire_interval")) | ||
| self._default_job_requeue_limit = int( | ||
| self._config.get("fq", "default_job_requeue_limit") | ||
| ) | ||
| self._key_prefix = redis_config["key_prefix"] | ||
| self._job_expire_interval = int(fq_config["job_expire_interval"]) | ||
| self._default_job_requeue_limit = int(fq_config["default_job_requeue_limit"]) | ||
|
|
||
| redis_connection_type = self._config.get("redis", "conn_type") | ||
| db = self._config.get("redis", "db") | ||
| redis_connection_type = redis_config["conn_type"] | ||
| db = redis_config["db"] |
There was a problem hiding this comment.
_initialize() indexes directly into fq_config / redis_config for required keys (e.g., redis_config["key_prefix"], fq_config["job_expire_interval"]). If a caller omits a key, this will raise a raw KeyError instead of an FQException, which makes misconfiguration harder to diagnose and is inconsistent with the rest of the error surface. Consider explicitly validating required keys (and basic types) up front and raising FQException with a clear path like redis.key_prefix / fq.job_expire_interval when missing/invalid.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/fq/queue.py (1)
67-72: Consider wrappingint()conversions to provide clearer error messages.Direct
int()calls on config values will raiseValueErrorwith a generic message if the value is not convertible. Since these are user-provided config values, a friendlier error would help debugging.💡 Optional: Add validation helper for integer config values
+def _get_int_config(config, key, context): + try: + return int(config[key]) + except (ValueError, TypeError) as e: + raise FQException(f"{context}.{key} must be an integer") from e + async def _initialize(self): """Read the FQ configuration and set up redis + Lua scripts.""" fq_config = self.config["fq"] redis_config = self.config["redis"] self._key_prefix = redis_config["key_prefix"] - self._job_expire_interval = int(fq_config["job_expire_interval"]) - self._default_job_requeue_limit = int(fq_config["default_job_requeue_limit"]) + self._job_expire_interval = _get_int_config(fq_config, "job_expire_interval", "fq") + self._default_job_requeue_limit = _get_int_config(fq_config, "default_job_requeue_limit", "fq")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fq/queue.py` around lines 67 - 72, Wrap the direct int() conversions for fq_config["job_expire_interval"] and fq_config["default_job_requeue_limit"] in a small validation step that catches ValueError and raises a clearer, contextual error; e.g., create or use a helper like validate_int_config(name, value, default=None) and replace the assignments to self._job_expire_interval and self._default_job_requeue_limit so they call the helper (referencing fq_config and the keys "job_expire_interval"/"default_job_requeue_limit"); ensure the helper includes the config key name and original invalid value in the raised exception message to aid debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/fq/queue.py`:
- Around line 67-72: Wrap the direct int() conversions for
fq_config["job_expire_interval"] and fq_config["default_job_requeue_limit"] in a
small validation step that catches ValueError and raises a clearer, contextual
error; e.g., create or use a helper like validate_int_config(name, value,
default=None) and replace the assignments to self._job_expire_interval and
self._default_job_requeue_limit so they call the helper (referencing fq_config
and the keys "job_expire_interval"/"default_job_requeue_limit"); ensure the
helper includes the config key name and original invalid value in the raised
exception message to aid debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0011d409-dabd-4f97-b5b1-b9ce07b60630
📒 Files selected for processing (8)
README.mdsrc/fq/default.confsrc/fq/queue.pytests/config.pytests/test.conftests/test_edge_cases.pytests/test_func.pytests/test_queue.py
💤 Files with no reviewable changes (2)
- src/fq/default.conf
- tests/test.conf
Strengthens configuration validation by raising descriptive errors for missing or invalid config values during object construction, ensuring misconfigurations are caught early. Updates tests to reflect the new validation logic and error paths, improving clarity and robustness of error handling.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/fq/queue.py (1)
156-159: Consider simplifying the clustered flag handling.The current pattern reads
clusteredtwice when it exists:if "clustered" in redis_config: isclustered = redis_config["clustered"]This is correct but could be slightly cleaner using
.get()with a default.♻️ Optional simplification
- isclustered = False - if "clustered" in redis_config: - isclustered = redis_config["clustered"] + isclustered = redis_config.get("clustered", False)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fq/queue.py` around lines 156 - 159, Replace the explicit membership check and assignment for the clustered flag with a single get call: instead of testing "clustered" in redis_config and then setting isclustered, set isclustered = redis_config.get("clustered", False) in the branch handling redis_connection_type == "tcp_sock" (referencing redis_connection_type, isclustered, and redis_config).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/fq/queue.py`:
- Around line 156-159: Replace the explicit membership check and assignment for
the clustered flag with a single get call: instead of testing "clustered" in
redis_config and then setting isclustered, set isclustered =
redis_config.get("clustered", False) in the branch handling
redis_connection_type == "tcp_sock" (referencing redis_connection_type,
isclustered, and redis_config).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc84d692-0c37-43bd-87aa-e6a6ad1177d3
📒 Files selected for processing (4)
src/fq/queue.pytests/test_edge_cases.pytests/test_func.pytests/test_queue.py
Updates configuration handling to use a Python mapping instead of an INI file, simplifying initialization and validation. Removes all file-based config logic, adjusts documentation and tests accordingly, and improves type safety for clustered Redis config. Enhances maintainability and reduces test setup complexity.
Summary by CodeRabbit
Documentation
Refactor
Tests
Bug Fixes