Convert config kept in TypedDict into a dataclass - closes #1225#1241
Convert config kept in TypedDict into a dataclass - closes #1225#1241
Conversation
WalkthroughConverts pytest-postgresql configuration from a TypedDict/dictionary to a frozen dataclass Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pytest_postgresql/config.py (1)
11-27: Consider making the dataclass frozen for immutability.The
PostgreSQLConfigdataclass correctly defines all configuration fields. However, configuration objects typically benefit from immutability to prevent accidental modifications.Apply this diff to make the dataclass immutable:
-@dataclass +@dataclass(frozen=True) class PostgreSQLConfig: """PostgreSQL Config."""
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
newsfragments/1225.feature.rst(1 hunks)pytest_postgresql/config.py(3 hunks)pytest_postgresql/factories/client.py(1 hunks)pytest_postgresql/factories/noprocess.py(1 hunks)pytest_postgresql/factories/process.py(6 hunks)tests/examples/test_assert_port_search_count_is_ten.py(1 hunks)tests/test_executor.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pytest_postgresql/factories/noprocess.py (1)
pytest_postgresql/janitor.py (1)
load(109-127)
pytest_postgresql/factories/process.py (2)
pytest_postgresql/config.py (2)
PostgreSQLConfig(12-27)get_config(30-55)pytest_postgresql/exceptions.py (1)
ExecutableMissingException(4-5)
🪛 Ruff (0.14.8)
tests/test_executor.py
60-60: Probable insecure usage of temporary file or directory: "/tmp/error"
(S108)
62-62: Probable insecure usage of temporary file or directory: "/tmp/version.error.log"
(S108)
🔇 Additional comments (14)
newsfragments/1225.feature.rst (1)
1-1: LGTM!The newsfragment accurately describes the configuration migration from TypedDict to dataclass.
pytest_postgresql/factories/client.py (1)
73-73: LGTM!The migration from dictionary-style to attribute access for
drop_test_databaseis correct and aligns with the newPostgreSQLConfigdataclass structure.tests/examples/test_assert_port_search_count_is_ten.py (1)
14-14: LGTM!The test correctly uses attribute access for
port_search_count, consistent with thePostgreSQLConfigdataclass migration.tests/test_executor.py (3)
54-64: LGTM!The test correctly migrates from dictionary-style to attribute access for configuration fields (
port,exec,host,unixsocketdir,startparams). The test logic remains unchanged.
87-96: LGTM!The configuration access has been correctly updated to use attributes (
host,unixsocketdir,startparams) instead of dictionary indexing, maintaining the test's original behaviour.
112-121: LGTM!The migration to attribute-based configuration access (
host,unixsocketdir,startparams) is correct and preserves the test's functionality.pytest_postgresql/factories/noprocess.py (1)
69-76: LGTM!All configuration accesses have been correctly migrated from dictionary-style to attribute access (
host,port,user,password,dbname,options,load,drop_test_database). The factory logic and defaults remain unchanged.pytest_postgresql/config.py (1)
30-55: LGTM!The migration from returning a dictionary to constructing and returning a
PostgreSQLConfigdataclass instance is correctly implemented. All configuration fields are properly assigned, and the type annotation on line 37 forload_pathsimproves type safety.pytest_postgresql/factories/process.py (6)
39-50: LGTM!The function correctly accepts
PostgreSQLConfigand uses attribute access (config.exec) instead of dictionary indexing. The executable discovery logic remains unchanged.
53-57: LGTM!The port selection function correctly uses
PostgreSQLConfigand accesses the port viaconfig.port. The logic for finding an unused port is preserved.
118-119: LGTM!Configuration fields
dbnameandloadare correctly accessed using attribute syntax, consistent with thePostgreSQLConfigdataclass structure.
141-147: LGTM!The
port_search_countis correctly accessed via attribute syntax from the configuration dataclass.
154-165: LGTM!All configuration attributes (
host,user,password,options,unixsocketdir,startparams,postgres_options) are correctly accessed using attribute syntax when constructing thePostgreSQLExecutor. The factory parameter fallback pattern is preserved.
177-178: LGTM!The
drop_test_databaseconfiguration is correctly accessed as an attribute, maintaining the existing conditional database drop logic.
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @fizyk. * #1241 (comment) The following files were modified: * `pytest_postgresql/config.py` * `pytest_postgresql/factories/client.py` * `pytest_postgresql/factories/noprocess.py` * `pytest_postgresql/factories/process.py` * `tests/test_executor.py`
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
newsfragments/1225.feature.rst(1 hunks)pytest_postgresql/config.py(3 hunks)pytest_postgresql/factories/client.py(1 hunks)pytest_postgresql/factories/noprocess.py(1 hunks)pytest_postgresql/factories/process.py(6 hunks)tests/examples/test_assert_port_search_count_is_ten.py(1 hunks)tests/test_executor.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- newsfragments/1225.feature.rst
- pytest_postgresql/factories/client.py
🧰 Additional context used
🧬 Code graph analysis (2)
pytest_postgresql/factories/noprocess.py (1)
pytest_postgresql/janitor.py (1)
load(109-127)
pytest_postgresql/factories/process.py (4)
pytest_postgresql/config.py (2)
PostgreSQLConfig(12-27)get_config(30-55)pytest_postgresql/exceptions.py (1)
ExecutableMissingException(4-5)pytest_postgresql/executor.py (1)
PostgreSQLExecutor(44-240)pytest_postgresql/janitor.py (2)
DatabaseJanitor(21-165)load(109-127)
🪛 Ruff (0.14.8)
tests/test_executor.py
60-60: Probable insecure usage of temporary file or directory: "/tmp/error"
(S108)
62-62: Probable insecure usage of temporary file or directory: "/tmp/version.error.log"
(S108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: postgresql_18 / postgres (3.10)
- GitHub Check: postgresql_18 / postgres (pypy-3.11)
- GitHub Check: postgresql_18 / postgres (3.11)
🔇 Additional comments (11)
pytest_postgresql/config.py (2)
11-27: LGTM! Well-structured dataclass definition.The frozen dataclass is an excellent choice for immutable configuration. All fields are properly typed, and the structure is clear.
37-55: LGTM! Clean construction of the config dataclass.The explicit type annotation for
load_pathsimproves code clarity, and the dataclass construction includes all necessary fields with proper type handling (e.g., int conversion for port_search_count).tests/examples/test_assert_port_search_count_is_ten.py (1)
11-14: LGTM! Correct migration to attribute access.The change from dictionary-style access to attribute access is correct and aligns with the new
PostgreSQLConfigdataclass implementation.tests/test_executor.py (3)
51-68: LGTM! Proper migration to attribute access.All configuration accesses have been correctly updated to use the new dataclass attributes (
config.port,config.exec,config.host,config.unixsocketdir,config.startparams).
72-96: LGTM! Consistent attribute access pattern.The test correctly uses attribute access for all configuration values (
config.host,config.unixsocketdir,config.startparams), maintaining consistency with the dataclass-based approach.
99-121: LGTM! Attribute access correctly applied.The configuration attributes are properly accessed throughout the test, consistent with the
PostgreSQLConfigdataclass implementation.pytest_postgresql/factories/noprocess.py (1)
68-99: LGTM! Consistent attribute-based configuration access.All configuration values are correctly accessed via attributes (
config.host,config.port,config.user,config.password,config.dbname,config.options,config.load,config.drop_test_database), properly migrating from dictionary-style access to the new dataclass approach.pytest_postgresql/factories/process.py (4)
31-31: LGTM! Proper import of the new config type.The import correctly includes both
PostgreSQLConfigandget_config, supporting the migration to dataclass-based configuration.
39-50: LGTM! Function signature and logic correctly updated.The
_pg_exefunction signature now acceptsPostgreSQLConfiginstead of a dictionary, and correctly accessesconfig.execusing attribute notation.
53-57: LGTM! Proper signature update and attribute access.The
_pg_portfunction signature correctly acceptsPostgreSQLConfig, and the implementation properly usesconfig.portwith attribute access.
107-182: LGTM! Comprehensive migration to dataclass-based configuration.All configuration attributes are correctly accessed throughout the fixture (
config.dbname,config.load,config.port_search_count,config.host,config.user,config.password,config.options,config.unixsocketdir,config.startparams,config.postgres_options,config.drop_test_database). The migration from dictionary-style to attribute-based access is complete and consistent.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pytest_postgresql/config.py (1)
44-44: Add validation forport_search_countinteger conversion.The
int()conversion could raise aValueErrorif the option value is not a valid integer string. Whilst this would likely be caught during test setup, it would be more user-friendly to provide a clear error message.Consider adding validation:
- port_search_count=int(get_postgresql_option("port_search_count")), + port_search_count=_parse_port_search_count(get_postgresql_option("port_search_count")),And add a helper function:
def _parse_port_search_count(value: Any) -> int: """Parse port_search_count value, ensuring it's a valid integer.""" try: return int(value) except (ValueError, TypeError) as exc: raise ValueError( f"Invalid port_search_count value: {value!r}. Must be an integer." ) from exc
🧹 Nitpick comments (1)
pytest_postgresql/config.py (1)
11-27: Consider expanding the docstring.Whilst the current implementation is solid, the docstring could be more descriptive. Consider documenting what the configuration represents or listing key attributes.
Example:
@dataclass(frozen=True) class PostgreSQLConfig: - """PostgreSQL Config.""" + """ + PostgreSQL configuration for pytest-postgresql fixtures. + + Holds all configuration options for PostgreSQL process execution, + connection parameters, and database setup. + """
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pytest_postgresql/config.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: postgresql_18 / postgres (3.12)
- GitHub Check: postgresql_18 / postgres (pypy-3.11)
- GitHub Check: postgresql_18 / postgres (3.13)
🔇 Additional comments (2)
pytest_postgresql/config.py (2)
3-5: LGTM!The new imports are appropriate for the dataclass-based configuration implementation.
58-68: LGTM!The
detect_pathsfunction correctly handles path conversion with appropriate type hints.
|
I appreciate the change, however it felt odd to have this as a "feature" instead of a "breaking change" - which, given inclusion in 8.0.0 is totally fine per SemVer rules, thanks for using those! I was only surprised that this wasn't added to the list like the others were. Callers of Again, I'm not sad about the change or its inclusion in the release - rather that it wasn't marked as a breaking change. Thanks for your continued work on a powerful testing library! |
|
@miketheman Sorry! 🤦🏻 You're right, it should be breaking change. Haven't thought about it this way. As for coderabbit, not sure, in one instance it pointed out a bad category for a newsfragment in one of my other libraries, so maybe with the new rules I placed for him in here, it'll take a look at those changes as well. Or maybe I'll have to extend them a bit. |
2 participants
Chore that needs to be done:
pipenv run towncrier create [issue_number].[type].rstTypes are defined in the pyproject.toml, issue_number either from issue tracker or the Pull request number
Summary by CodeRabbit
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.