REFACTOR: Clearing init.py file #386
Conversation
… jahnvi/pyformat_param_style
There was a problem hiding this comment.
Pull request overview
This pull request refactors the mssql_python package to improve maintainability and add support for the pyformat parameter style alongside the existing qmark style. The changes focus on three main areas: automatic constant exports, modularization of decimal separator configuration, and enhanced parameter style handling.
Key Changes:
- Automatic export of SQL and driver constants at module level, eliminating manual constant definitions in
__init__.py - Decimal separator configuration logic extracted to a new
decimal_config.pymodule with factory pattern - New parameter conversion system supporting both
qmark(?) andpyformat(%(name)s) styles with automatic detection and conversion
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_015_pyformat_parameters.py |
Comprehensive test suite for new pyformat parameter style support (1882 lines of tests) |
tests/test_001_globals.py |
Updated paramstyle assertion from "qmark" to "pyformat" |
mssql_python/parameter_helper.py |
New module implementing parameter parsing, conversion, and auto-detection logic |
mssql_python/decimal_config.py |
New module encapsulating decimal separator configuration with factory pattern |
mssql_python/cursor.py |
Integrated parameter conversion into execute() and executemany() methods |
mssql_python/constants.py |
Added automatic constant export logic and helper function |
mssql_python/__init__.py |
Removed manual constant definitions, changed paramstyle to "pyformat", integrated decimal_config |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Test multiple parameters as tuple (line 1257).""" | ||
| cursor = db_connection.cursor() | ||
| # Multiple params as tuple - should use actual_params = parameters (line 1257) | ||
| cursor.execute("SELECT ?, ?", 10, 20) |
There was a problem hiding this comment.
The execute method is being called with three positional arguments after the SQL string. According to the cursor.py signature which uses *parameters, this should work but may be confusing. Consider using tuple notation for clarity: cursor.execute("SELECT ?, ?", (10, 20)) instead of cursor.execute("SELECT ?, ?", 10, 20).
| try: | ||
| if set_in_cpp_func is not None: | ||
| set_in_cpp_func(separator) | ||
| except ImportError: | ||
| # Handle case where ddbc_bindings is not available | ||
| pass | ||
|
|
||
|
|
There was a problem hiding this comment.
The except block catches ImportError but the comment says "Handle case where ddbc_bindings is not available". However, if DDBCSetDecimalSeparator(separator) raises an ImportError for any other reason (e.g., a missing dependency within ddbc_bindings), it will be silently ignored. Consider being more specific about what's being caught or re-raising unexpected errors.
| try: | |
| if set_in_cpp_func is not None: | |
| set_in_cpp_func(separator) | |
| except ImportError: | |
| # Handle case where ddbc_bindings is not available | |
| pass | |
| if set_in_cpp_func is not None: | |
| set_in_cpp_func(separator) |
| """ | ||
| # Support %% escaping - replace %% with a placeholder before parsing | ||
| # This allows users to have literal % in their SQL | ||
| escaped_sql = sql.replace("%%", "\x00ESCAPED_PERCENT\x00") |
There was a problem hiding this comment.
The placeholder string "\x00ESCAPED_PERCENT\x00" could potentially exist in user SQL queries, although it's extremely unlikely. Consider using a more unique placeholder or a guaranteed unique token (e.g., using uuid or a longer null-byte sequence) to avoid any theoretical collision.
| ] | ||
|
|
||
| # Clean up temporary variables | ||
| del _module_globals, _exported_names, _name, _member, _DDBC_PUBLIC_API |
There was a problem hiding this comment.
The cleanup at line 612 deletes temporary variables including _name and _member, but these are loop variables from the for loops. In Python 3, loop variables persist after the loop completes, so this deletion is valid. However, _DDBC_PUBLIC_API is deleted but it's a set that was defined at the module level and could be useful for introspection or testing. Consider keeping it or making it part of all if it's meant to be internal.
| del _module_globals, _exported_names, _name, _member, _DDBC_PUBLIC_API | |
| del _module_globals, _exported_names, _name, _member |
| # Read-Only | ||
| apilevel: str = "2.0" | ||
| paramstyle: str = "qmark" | ||
| paramstyle: str = "pyformat" |
There was a problem hiding this comment.
The paramstyle is changed from "qmark" to "pyformat", but this is a breaking API change. According to PEP 249, paramstyle indicates which parameter marker format is supported. Changing this from "qmark" (?) to "pyformat" (%(name)s) could break existing code that relies on this value to determine how to format parameters. While the implementation now supports both styles automatically, the paramstyle should reflect the primary/preferred style or be documented as supporting multiple styles. Consider if this should be "pyformat" or remain "qmark" with documentation noting that both are supported.
| paramstyle: str = "pyformat" | |
| # PEP 249 paramstyle: keep "qmark" for backward compatibility; implementation also supports "pyformat". | |
| paramstyle: str = "qmark" |
| """Test three parameters (line 1257 else branch).""" | ||
| cursor = db_connection.cursor() | ||
| # Three params - goes through else: actual_params = parameters | ||
| cursor.execute("SELECT ?, ?, ?", 1, 2, 3) |
There was a problem hiding this comment.
The execute method is being called with four positional arguments after the SQL string. According to the cursor.py signature which uses *parameters, this should work but may be confusing. Consider using tuple notation for clarity: cursor.execute("SELECT ?, ?, ?", (1, 2, 3)) instead of cursor.execute("SELECT ?, ?, ?", 1, 2, 3).
| if isinstance(parameters, tuple) and len(parameters) == 1: | ||
| # Could be either (value,) for single param or ((tuple),) for nested | ||
| # Check if it's a nested container | ||
| if isinstance(parameters[0], (tuple, list, dict)): | ||
| actual_params = parameters[0] | ||
| else: | ||
| actual_params = parameters |
There was a problem hiding this comment.
The parameter wrapping logic has a subtle issue: when a single-item tuple like (42,) is passed, the code at line 1250 checks if parameters[0] is a tuple/list/dict. Since (42,) contains an int, it will take the else branch at line 1253 and set actual_params = parameters, which is correct. However, for a case like execute("SELECT ?", ((1,2),)), where parameters[0] is (1,2), it will unwrap to actual_params = (1,2), treating it as two parameters instead of one tuple parameter. This could break legitimate use cases where users want to pass a single tuple as a parameter value.
|
|
||
| # Auto-detect and convert parameter style if needed | ||
| # Supports both qmark (?) and pyformat (%(name)s) | ||
| # Note: parameters is always a tuple due to *parameters in method signature |
There was a problem hiding this comment.
The comment says "parameters is always a tuple due to *parameters in method signature" but this isn't accurate. The method signature uses *parameters which collects variadic arguments into a tuple, but this means an empty call would result in an empty tuple, not necessarily "always a tuple". The code correctly handles this with the if parameters: check, but the comment is slightly misleading.
| # Note: parameters is always a tuple due to *parameters in method signature | |
| # Note: parameters comes from *parameters in the method signature, so it is a | |
| # tuple of positional arguments (which may be empty) |
| # Import all module-level constants from constants module | ||
| from .constants import * # noqa: F401, F403 | ||
|
|
||
|
|
There was a problem hiding this comment.
The wildcard import from .constants import * makes it difficult to track which names are being imported into the module namespace. While all is defined in constants.py to control exports, this can make the code harder to maintain and debug. Additionally, the # noqa: F401, F403 suppresses linting warnings about unused imports and undefined names from wildcard imports. Consider explicitly importing the needed constants or at least document what's being imported.
| # Import all module-level constants from constants module | |
| from .constants import * # noqa: F401, F403 | |
| # Import constants module and re-export its public names defined in __all__ | |
| from . import constants as _constants | |
| for _name in getattr(_constants, "__all__", ()): | |
| setattr(sys.modules[__name__], _name, getattr(_constants, _name)) |
| if not isinstance(first_row, (tuple, list, dict)): | ||
| # First row is a single non-sequence parameter - wrap all rows | ||
| seq_of_parameters = [(param,) for param in seq_of_parameters] | ||
| first_row = seq_of_parameters[0] |
There was a problem hiding this comment.
The code wraps single non-sequence parameters for backward compatibility, but there's a potential issue: if a user passes an iterator that yields single values (not tuples/lists/dicts), the code will consume the first element to check its type, then try to iterate again to wrap all elements. This could fail if seq_of_parameters is a non-reusable iterator (like a generator). Consider converting to a list first or using itertools.tee to avoid iterator exhaustion.
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/constants.pyLines 513-521 513
514 Returns:
515 dict: Dictionary mapping constant names to their integer values
516 """
! 517 return {name: member.value for name, member in GetInfoConstants.__members__.items()}
518
519
520 # Automatically export selected enum constants as module-level attributes
521 # This allows: from mssql_python import SQL_VARCHARmssql_python/cursor.pyLines 1259-1267 1259
1260 # Convert back to list format expected by the binding code
1261 # detect_and_convert_parameters always returns None, tuple, or list
1262 if converted_params is None:
! 1263 parameters = []
1264 else:
1265 parameters = list(converted_params)
1266 else:
1267 parameters = []mssql_python/decimal_config.pyLines 7-15 7
8 from typing import TYPE_CHECKING
9
10 if TYPE_CHECKING:
! 11 from mssql_python.helpers import Settings
12
13
14 def setDecimalSeparator(separator: str, settings: "Settings", set_in_cpp_func=None) -> None:
15 """Lines 44-52 44 raise ValueError("Whitespace characters are not allowed as decimal separators")
45
46 # Check for specific disallowed characters
47 if separator in ["\t", "\n", "\r", "\v", "\f"]:
! 48 raise ValueError(
49 f"Control character '{repr(separator)}' is not allowed as a decimal separator"
50 )
51
52 # Set in Python side settingsLines 56-66 56 # Set the initial decimal separator in C++
57 try:
58 if set_in_cpp_func is not None:
59 set_in_cpp_func(separator)
! 60 except ImportError:
61 # Handle case where ddbc_bindings is not available
! 62 pass
63
64
65 def getDecimalSeparator(settings: "Settings") -> str:
66 """Lines 94-104 94
95 # Set the initial decimal separator in C++
96 DDBCSetDecimalSeparator(settings.decimal_separator)
97 cpp_binding = DDBCSetDecimalSeparator
! 98 except ImportError:
99 # Handle case where ddbc_bindings is not available
! 100 cpp_binding = None
101
102 def setter(separator: str) -> None:
103 """
104 Sets the decimal separator character used when parsing NUMERIC/DECIMAL values📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.hpp: 58.8%
mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.row.py: 66.2%
mssql_python.helpers.py: 67.5%
mssql_python.pybind.ddbc_bindings.cpp: 69.3%
mssql_python.pybind.ddbc_bindings.h: 71.7%
mssql_python.pybind.connection.connection.cpp: 73.6%
mssql_python.__init__.py: 73.9%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 79.6%🔗 Quick Links
|
Work Item / Issue Reference
Summary
This pull request makes several significant improvements to the
mssql_pythonpackage, focusing on simplifying constant exports, modularizing decimal separator configuration, and enhancing parameter style handling for SQL execution. The changes improve maintainability, usability, and compatibility with Python DB-API conventions.Highlights:
Cursor.executeandCursor.executemanyis enhanced to support bothqmarkandpyformatstyles seamlessly.Constants Export and API Simplification
mssql_python/constants.py, replacing manual assignments in__init__.py. This makes constants likeSQL_VARCHARandSQL_DRIVER_NAMEdirectly accessible and maintains a clean and DRY codebase. A helper functionget_info_constants()is also provided for easy access to all GetInfo constants.Decimal Separator Configuration Refactor
__init__.pyto a new module,decimal_config.py. This module provides a factory to create getter/setter functions bound to the settings instance, and handles C++ binding initialization. This improves modularity and testability.Parameter Style Handling Improvements
Cursor.executeandCursor.executemanymethods now automatically detect and convert betweenqmark(?) andpyformat(%(name)s) parameter styles. This allows for greater flexibility and compatibility with various SQL parameterization patterns, including seamless support for both positional and named parameters.Minor Cleanups
__init__.pyto avoid circular dependencies and unnecessary code.