STYLE: Linting python files and making it fully typed #298
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This pull request enhances the mssql_python package by adding comprehensive type annotations throughout the codebase to improve type safety, IDE support, and code maintainability. The changes focus primarily on adding explicit type hints to functions, methods, and variables across Python modules and test files.
Key changes:
- Added type annotations to global variables, constants, and function signatures across all modules
- Improved code formatting by breaking long lines and standardizing quote usage (single to double quotes)
- Enhanced type stub file (
mssql_python.pyi) with more detailed type information
Reviewed Changes
Copilot reviewed 24 out of 26 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
mssql_python/__init__.py |
Added type annotations to global settings, constants, and module-level functions |
mssql_python/auth.py |
Added type hints for authentication processing functions with List and Dict types |
mssql_python/connection.py |
Enhanced Connection class with comprehensive type annotations for all methods and properties |
mssql_python/cursor.py |
Added type annotations to Cursor class methods and internal helper functions |
mssql_python/row.py |
Added type hints to Row class for improved type safety |
mssql_python/pooling.py |
Added type annotations to PoolingManager class attributes and methods |
mssql_python/constants.py |
Improved formatting of constant definitions and added spacing |
mssql_python/exceptions.py |
Added type hints to all exception classes and helper functions |
mssql_python/helpers.py |
Enhanced helper functions with comprehensive type annotations |
mssql_python/logging_config.py |
Added type hints to LoggingManager singleton class |
mssql_python/type.py |
Added type annotations to type constructor functions |
mssql_python/db_connection.py |
Enhanced connect function with type hints |
mssql_python/ddbc_bindings.py |
Improved code formatting and consistency |
mssql_python/mssql_python.pyi |
Significantly expanded type stub file with detailed annotations |
mssql_python/bcp_options.py |
File deleted (appears to be unused) |
tests/*.py |
Reformatted test files for consistency with black formatter |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/init.pyLines 66-76 66 # Set the initial decimal separator in C++
67 try:
68 from .ddbc_bindings import DDBCSetDecimalSeparator
69 DDBCSetDecimalSeparator(_settings.decimal_separator)
! 70 except ImportError:
71 # Handle case where ddbc_bindings is not available
! 72 DDBCSetDecimalSeparator = None
73
74
75 # New functions for decimal separator control
76 def setDecimalSeparator(separator: str) -> None:Lines 104-112 104 raise ValueError("Whitespace characters are not allowed as decimal separators")
105
106 # Check for specific disallowed characters
107 if separator in ["\t", "\n", "\r", "\v", "\f"]:
! 108 raise ValueError(
109 f"Control character '{repr(separator)}' is not allowed as a decimal separator"
110 )
111
112 # Set in Python side settingsmssql_python/auth.pyLines 30-39 30 DeviceCodeCredential,
31 InteractiveBrowserCredential,
32 )
33 from azure.core.exceptions import ClientAuthenticationError
! 34 except ImportError as e:
! 35 raise RuntimeError(
36 "Azure authentication libraries are not installed. "
37 "Please install with: pip install azure-identity azure-core"
38 ) from emssql_python/connection.pyLines 720-732 720 # Some drivers might return this as an integer memory address
721 # or other non-string format, so ensure we have a string
722 if not isinstance(escape_char, str):
723 # Default to backslash if not a string
! 724 escape_char = "\\"
725 self._searchescape = escape_char
726 except Exception as e:
727 # Log the exception for debugging, but do not expose sensitive info
! 728 log(
729 "warning",
730 "Failed to retrieve search escape character, using default '\\'. "
731 "Exception: %s",
732 type(e).__name__,Lines 730-738 730 "Failed to retrieve search escape character, using default '\\'. "
731 "Exception: %s",
732 type(e).__name__,
733 )
! 734 self._searchescape = "\\"
735 return self._searchescape
736
737 def cursor(self) -> Cursor:
738 """Lines 1018-1026 1018 "debug",
1019 "Automatically closed cursor after batch execution error",
1020 )
1021 except Exception as close_err:
! 1022 log(
1023 "warning",
1024 f"Error closing cursor after execution failure: {close_err}",
1025 )
1026 # Re-raise the original exceptionLines 1166-1174 1166 except UnicodeDecodeError:
1167 try:
1168 return actual_data.decode("latin1").rstrip("\0")
1169 except Exception as e:
! 1170 log(
1171 "error",
1172 "Failed to decode string in getinfo: %s. "
1173 "Returning None to avoid silent corruption.",
1174 e,Lines 1185-1193 1185 if byte_val in (b"Y"[0], b"y"[0], 1):
1186 return "Y"
1187 return "N"
1188 # If it's not a byte or we can't determine, default to 'N'
! 1189 return "N"
1190 elif is_numeric_type:
1191 # Handle numeric types based on length
1192 if isinstance(data, bytes):
1193 # Map byte length → signed int sizeLines 1238-1246 1238 pass
1239
1240 # Last resort: return as integer if all else fails
1241 try:
! 1242 return int.from_bytes(
1243 data[: min(length, 8)], "little", signed=True
1244 )
1245 except Exception:
1246 return 0Lines 1262-1283 1262 if isinstance(data, bytes):
1263 # Try to convert to string first
1264 try:
1265 return data[:length].decode("utf-8").rstrip("\0")
! 1266 except UnicodeDecodeError:
! 1267 pass
1268
1269 # Try to convert to int for short binary data
! 1270 try:
! 1271 if length <= 8:
! 1272 return int.from_bytes(data[:length], "little", signed=True)
! 1273 except Exception: # pylint: disable=broad-exception-caught
! 1274 pass
1275
1276 # Return as is if we can't determine
! 1277 return data
1278
! 1279 return data
1280
1281 return raw_result # Return as-is
1282
1283 def commit(self) -> None:Lines 1353-1368 1353 for cursor in cursors_to_close:
1354 try:
1355 if not cursor.closed:
1356 cursor.close()
! 1357 except Exception as e: # pylint: disable=broad-exception-caught
1358 # Collect errors but continue closing other cursors
1359 close_errors.append(f"Error closing cursor: {e}")
! 1360 log("warning", f"Error closing cursor: {e}")
1361
1362 # If there were errors closing cursors, log them but continue
1363 if close_errors:
! 1364 log(
1365 "warning",
1366 "Encountered %d errors while closing cursors",
1367 len(close_errors),
1368 )mssql_python/cursor.pyLines 273-281 273 if -exponent > num_digits:
274 int_str = ("0" * (-exponent - num_digits)) + int_str
275
276 if int_str == "":
! 277 int_str = "0"
278
279 # Convert decimal base-10 string to python int, then to 16 little-endian bytes
280 big_int = int(int_str)
281 byte_array = bytearray(16) # SQL_MAX_NUMERIC_LENLines 380-388 380 # Handle special values (NaN, Infinity, etc.)
381 if isinstance(exponent, str):
382 # For special values like 'n' (NaN), 'N' (sNaN), 'F' (Infinity)
383 # Return default precision and scale
! 384 precision = 38 # SQL Server default max precision
385 else:
386 # Calculate the SQL precision (same logic as _get_numeric_data)
387 if exponent >= 0:
388 precision = num_digits + exponentLines 770-778 770 if sql_type in (
771 ddbc_sql_const.SQL_DECIMAL.value,
772 ddbc_sql_const.SQL_NUMERIC.value,
773 ):
! 774 column_size = max(
775 1, min(int(column_size) if column_size > 0 else 18, 38)
776 )
777 decimal_digits = min(max(0, decimal_digits), column_size)Lines 992-1000 992
993 Raises:
994 StopIteration: When no more rows are available.
995 """
! 996 return next(self)
997
998 def execute( # pylint: disable=too-many-locals,too-many-branches,too-many-statements
999 self,
1000 operation: str,Lines 1037-1045 1037 ddbc_sql_const.SQL_ATTR_QUERY_TIMEOUT.value,
1038 timeout_value,
1039 )
1040 check_error(ddbc_sql_const.SQL_HANDLE_STMT.value, self.hstmt, ret)
! 1041 log("debug", f"Set query timeout to {timeout_value} seconds")
1042 except Exception as e: # pylint: disable=broad-exception-caught
1043 log("warning", f"Failed to set query timeout: {e}")
1044
1045 param_info = ddbc_bindings.ParamInfoLines 1128-1136 1128 # ODBC specification guarantees that column metadata is available immediately after
1129 # a successful SQLExecute/SQLExecDirect for the first result set
1130 ddbc_bindings.DDBCSQLDescribeCol(self.hstmt, column_metadata)
1131 self._initialize_description(column_metadata)
! 1132 except Exception as e: # pylint: disable=broad-exception-caught
1133 # If describe fails, it's likely there are no results (e.g., for INSERT)
1134 self.description = None
1135
1136 # Reset rownumber for new result set (only for SELECT statements)Lines 1148-1156 1148 if desc and desc[1] == uuid.UUID: # Column type code at index 1
1149 self._uuid_indices.append(i)
1150 # Verify we have complete description tuples (7 items per PEP-249)
1151 elif desc and len(desc) != 7:
! 1152 log(
1153 "warning",
1154 f"Column description at index {i} has incorrect tuple length: {len(desc)}",
1155 )
1156 self.rowcount = -1Lines 1188-1199 1188 column_metadata = []
1189 try:
1190 ddbc_bindings.DDBCSQLDescribeCol(self.hstmt, column_metadata)
1191 except InterfaceError as e:
! 1192 log("error", f"Driver interface error during metadata retrieval: {e}")
! 1193 except Exception as e: # pylint: disable=broad-exception-caught
1194 # Log the exception with appropriate context
! 1195 log(
1196 "error",
1197 f"Failed to retrieve column metadata: {e}. "
1198 f"Using standard ODBC column definitions instead.",
1199 )Lines 1218-1226 1218 # Define specialized fetch methods that use the custom mapping
1219 def fetchone_with_specialized_mapping():
1220 row = self._original_fetchone()
1221 if row is not None:
! 1222 merged_map = getattr(row, "_column_map", {}).copy()
1223 merged_map.update(specialized_mapping)
1224 row._column_map = merged_map
1225 return rowLines 1226-1234 1226
1227 def fetchmany_with_specialized_mapping(size=None):
1228 rows = self._original_fetchmany(size)
1229 for row in rows:
! 1230 merged_map = getattr(row, "_column_map", {}).copy()
1231 merged_map.update(specialized_mapping)
1232 row._column_map = merged_map
1233 return rowsLines 1234-1242 1234
1235 def fetchall_with_specialized_mapping():
1236 rows = self._original_fetchall()
1237 for row in rows:
! 1238 merged_map = getattr(row, "_column_map", {}).copy()
1239 merged_map.update(specialized_mapping)
1240 row._column_map = merged_map
1241 return rowsLines 1240-1251 1240 row._column_map = merged_map
1241 return rows
1242
1243 # Save original fetch methods
! 1244 if not hasattr(self, "_original_fetchone"):
! 1245 self._original_fetchone = self.fetchone # pylint: disable=attribute-defined-outside-init
! 1246 self._original_fetchmany = self.fetchmany # pylint: disable=attribute-defined-outside-init
! 1247 self._original_fetchall = self.fetchall # pylint: disable=attribute-defined-outside-init
1248
1249 # Use specialized mapping methods
1250 self.fetchone = fetchone_with_specialized_mapping
1251 self.fetchmany = fetchmany_with_specialized_mappingLines 1303-1311 1303 check_error(ddbc_sql_const.SQL_HANDLE_STMT.value, self.hstmt, ret)
1304
1305 # Use the helper method to prepare the result set
1306 return self._prepare_metadata_result_set()
! 1307 except Exception as e: # pylint: disable=broad-exception-caught
1308 self._reset_cursor()
1309 raise e
1310
1311 def procedures(self, procedure=None, catalog=None, schema=None):Lines 1715-1725 1715 ddbc_sql_const.SQL_ATTR_QUERY_TIMEOUT.value,
1716 timeout_value,
1717 )
1718 check_error(ddbc_sql_const.SQL_HANDLE_STMT.value, self.hstmt, ret)
! 1719 log("debug", f"Set query timeout to {self._timeout} seconds")
! 1720 except Exception as e: # pylint: disable=broad-exception-caught
! 1721 log("warning", f"Failed to set query timeout: {e}")
1722
1723 # Get sample row for parameter type detection and validation
1724 sample_row = (
1725 seq_of_parameters[0]Lines 1774-1782 1774 if sql_type in (
1775 ddbc_sql_const.SQL_DECIMAL.value,
1776 ddbc_sql_const.SQL_NUMERIC.value,
1777 ):
! 1778 column_size = max(
1779 1, min(int(column_size) if column_size > 0 else 18, 38)
1780 )
1781 decimal_digits = min(max(0, decimal_digits), column_size)Lines 1889-1897 1889 ddbc_sql_const.SQL_NUMERIC.value,
1890 ) and not isinstance(val, decimal.Decimal):
1891 try:
1892 processed_row[i] = decimal.Decimal(str(val))
! 1893 except Exception as e: # pylint: disable=broad-exception-caught
1894 raise ValueError(
1895 f"Failed to convert parameter at row {row}, column {i} to Decimal: {e}"
1896 ) from e
1897 processed_parameters.append(processed_row)Lines 2023-2031 2023 return [
2024 Row(self, self.description, row_data, column_map, settings_snapshot)
2025 for row_data in rows_data
2026 ]
! 2027 except Exception as e: # pylint: disable=broad-exception-caught
2028 # On error, don't increment rownumber - rethrow the error
2029 raise e
2030
2031 def fetchall(self) -> List[Row]:Lines 2064-2072 2064 return [
2065 Row(self, self.description, row_data, column_map, settings_snapshot)
2066 for row_data in rows_data
2067 ]
! 2068 except Exception as e: # pylint: disable=broad-exception-caught
2069 # On error, don't increment rownumber - rethrow the error
2070 raise e
2071
2072 def nextset(self) -> Union[bool, None]:Lines 2214-2222 2214 """
2215 if "closed" not in self.__dict__ or not self.closed:
2216 try:
2217 self.close()
! 2218 except Exception as e: # pylint: disable=broad-exception-caught
2219 # Don't raise an exception in __del__, just log it
2220 # If interpreter is shutting down, we might not have logging set up
2221 import sysLines 2222-2230 2222
2223 if sys and sys._is_finalizing():
2224 # Suppress logging during interpreter shutdown
2225 return
! 2226 log("debug", "Exception during cursor cleanup in __del__: %s", e)
2227
2228 def scroll(self, value: int, mode: str = "relative") -> None: # pylint: disable=too-many-branches
2229 """
2230 Scroll using SQLFetchScroll only, matching test semantics:Lines 2429-2440 2429 return self._prepare_metadata_result_set(
2430 fallback_description=fallback_description
2431 )
2432
! 2433 except Exception as e: # pylint: disable=broad-exception-caught
2434 # Log the error and re-raise
2435 log("error", f"Error executing tables query: {e}")
! 2436 raise
2437
2438 def callproc(
2439 self, procname: str, parameters: Optional[Sequence[Any]] = None
2440 ) -> Optional[Sequence[Any]]:mssql_python/ddbc_bindings.pyLines 89-98 89 architecture = "amd64"
90
91 # Validate supported platforms
92 if platform_name not in ["windows", "darwin", "linux"]:
! 93 supported_platforms = ["windows", "darwin", "linux"]
! 94 raise ImportError(
95 f"Unsupported platform '{platform_name}' for mssql-python; expected one "
96 f"of {supported_platforms}"
97 )Lines 97-105 97 )
98
99 # Determine extension based on platform
100 if platform_name == "windows":
! 101 extension = ".pyd"
102 else: # macOS or Linux
103 extension = ".so"
104
105 # Find the specifically matching module fileLines 108-116 108 module_path = os.path.join(module_dir, expected_module)
109
110 if not os.path.exists(module_path):
111 # Fallback to searching for any matching module if the specific one isn't found
! 112 module_files = [
113 f
114 for f in os.listdir(module_dir)
115 if f.startswith("ddbc_bindings.") and f.endswith(extension)
116 ]Lines 114-127 114 for f in os.listdir(module_dir)
115 if f.startswith("ddbc_bindings.") and f.endswith(extension)
116 ]
117 if not module_files:
! 118 raise ImportError(
119 f"No ddbc_bindings module found for {python_version}-{architecture} "
120 f"with extension {extension}"
121 )
122 module_path = os.path.join(module_dir, module_files[0])
! 123 print(
124 f"Warning: Using fallback module file {module_files[0]} instead of "
125 f"{expected_module}"
126 )mssql_python/exceptions.pyLines 526-534 526 string_third = string_second[string_second.index("]") + 1 :]
527 return string_first + string_third
528 except Exception as e:
529 if logger:
! 530 logger.error("Error while truncating error message: %s", e)
531 return error_message
532
533
534 def raise_exception(sqlstate: str, ddbc_error: str) -> None:mssql_python/helpers.pyLines 53-61 53 final_connection_attributes.insert(0, driver_name)
54 connection_str = ";".join(final_connection_attributes)
55
56 except Exception as e:
! 57 raise ValueError(
58 "Invalid connection string, Please follow the format: "
59 "Server=server_name;Database=database_name;UID=user_name;PWD=password"
60 ) from eLines 184-192 184 def _sanitize_for_logging(input_val: Any, max_length: int = max_log_length) -> str:
185 if not isinstance(input_val, str):
186 try:
187 input_val = str(input_val)
! 188 except (TypeError, ValueError):
189 return "<non-string>"
190
191 # Allow alphanumeric, dash, underscore, and dot
192 sanitized = re.sub(r"[^\w\-\.]", "", input_val)Lines 305-313 305 *args: Arguments for message formatting
306 """
307 current_logger = get_logger()
308 if current_logger:
! 309 getattr(current_logger, level)(message, *args)
310
311
312 # Settings functionality moved here to avoid circular importsLines 318-327 318 # Get the locale setting once during module initialization
319 locale_separator = locale.localeconv()["decimal_point"]
320 if locale_separator and len(locale_separator) == 1:
321 _default_decimal_separator = locale_separator
! 322 except (AttributeError, KeyError, TypeError, ValueError):
! 323 pass # Keep the default "." if locale access fails
324
325
326 class Settings:
327 """mssql_python/row.pyLines 126-134 126 value = processed_values[i]
127 if isinstance(value, str):
128 try:
129 # Remove braces if present
! 130 clean_value = value.strip("{}")
131 processed_values[i] = uuid.UUID(clean_value)
132 except (ValueError, AttributeError):
133 pass # Keep original if conversion fails
134 # Fallback to scanning all columns if indices weren't pre-identifiedLines 142-150 142 sql_type = description[i][1]
143 if sql_type == -11: # SQL_GUID
144 if isinstance(value, str):
145 try:
! 146 processed_values[i] = uuid.UUID(value.strip("{}"))
147 except (ValueError, AttributeError):
148 pass
149 # When native_uuid is False, convert UUID objects to strings
150 else:Lines 215-226 215 value_bytes = value.to_bytes(
216 byte_size, byteorder="little", signed=True
217 )
218 converted_values[i] = converter(value_bytes)
! 219 except OverflowError:
220 # Log specific overflow error with details to help diagnose the issue
! 221 if hasattr(self._cursor, "log"):
! 222 self._cursor.log(
223 "warning",
224 f"Integer overflow: value {value} does not fit in "
225 f"{byte_size} bytes for SQL type {sql_type}",
226 )Lines 230-238 230 converted_values[i] = converter(value)
231 except Exception as e:
232 # Log the exception for debugging without leaking sensitive data
233 if hasattr(self._cursor, "log"):
! 234 self._cursor.log(
235 "warning",
236 f"Exception in output converter: {type(e).__name__} "
237 f"for SQL type {sql_type}",
238 )📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.ddbc_bindings.cpp: 70.5%
mssql_python.row.py: 77.9%
mssql_python.pybind.connection.connection_pool.cpp: 78.9%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection.cpp: 80.1%
mssql_python.connection.py: 82.9%
mssql_python.cursor.py: 83.6%
mssql_python.auth.py: 87.1%
mssql_python.pooling.py: 87.7%
mssql_python.exceptions.py: 92.1%🔗 Quick Links
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Work Item / Issue Reference
Summary
This pull request refactors the
mssql_pythonpackage to improve type safety and code clarity by adding explicit type annotations throughout the codebase. The changes mainly focus on the__init__.pyandauth.pymodules, updating function signatures, global variables, and constants to use Python type hints. This will help with static analysis, improve IDE support, and make the code easier to understand and maintain.Type Annotation Improvements
mssql_python/__init__.py, including SQL constants and configuration settings.mssql_python/auth.pyto use type hints for parameters and return types, such as changing rawlist/dictusage toList[str],Dict[int, bytes], andOptional[...].Code Quality and Consistency
Settingsand custom module classes.Minor Logic and Readability Enhancements
auth.py.These changes collectively enhance the maintainability and robustness of the codebase by leveraging Python's type system and improving code readability.