-
Notifications
You must be signed in to change notification settings - Fork 18
FEAT: Adding lowercase for global variable #187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request introduces a global lowercase
flag to the mssql_python
package, allowing users to control whether column names in query results are automatically converted to lowercase. The feature includes updates to the global settings infrastructure, cursor and row classes to support the lowercase functionality, and comprehensive tests.
- Added global
lowercase
flag with supporting Settings class and accessor functions - Modified Cursor and Row classes to respect the lowercase setting for column name handling
- Updated Row class to support case-insensitive attribute access when lowercase mode is enabled
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
mssql_python/init.py | Introduces Settings class, global lowercase flag, and get_settings() accessor |
mssql_python/cursor.py | Updates cursor to read global lowercase setting and apply it to column descriptions |
mssql_python/row.py | Modifies Row class to support case-insensitive attribute access and updated constructor |
tests/test_001_globals.py | Adds test to verify default value of lowercase flag |
tests/test_004_cursor.py | Adds comprehensive test for lowercase functionality with proper cleanup |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
def get_settings(): | ||
return _settings | ||
|
||
lowercase = _settings.lowercase # Default is False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The global lowercase
variable is a static copy of _settings.lowercase
and won't reflect changes made to the settings object. This means mssql_python.lowercase = True
assignments won't update the actual settings. Consider making lowercase
a property that references the settings object directly.
lowercase = _settings.lowercase # Default is False | |
# lowercase is now a dynamic property referencing _settings.lowercase |
Copilot uses AI. Check for mistakes.
self.description = None | ||
return | ||
import mssql_python | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import statement should be moved to the top of the file with other imports rather than being placed inside the method. This follows Python best practices and improves readability.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with this.
# Get column name - lowercase it if the lowercase flag is set | ||
column_name = col["ColumnName"] | ||
|
||
if mssql_python.lowercase: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are direct accesses to the global lowercase
variable in mssql_python/cursor.py
. For better maintainability and to avoid issues with mutable global state, please ensure all access to this variable goes through the get_settings()
accessor. This will centralize state management and help prevent subtle bugs or inconsistencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment.. Please resolve. Rest all looks good
col_name = col_desc[0] # Name is first item in description tuple | ||
column_map[col_name] = i | ||
|
||
self._column_map = column_map | ||
|
||
def __getitem__(self, index): | ||
"""Allow accessing by numeric index: row[0]""" | ||
return self._values[index] | ||
|
||
def __getattr__(self, name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation in row.py does indeed have conditional case-insensitive behavior that should be documented for clarity.
The case-insensitive behavior depends on how _column_map is populated, which happens during cursor initialization when the global lowercase flag is enabled. This creates an important behavioral difference that users should be aware of.
Update the docstring to clearly document this behavior:
Note: Case sensitivity depends on the global 'lowercase' setting: - When lowercase=True: Column names are stored in lowercase, enabling case-insensitive attribute access (e.g., row.NAME, row.name, row.Name all work) - When lowercase=False (default): Column names preserve original casing, requiring exact case matching for attribute access
Additionally, consider updating the class-level docstring to mention this behavior:
`
A row of data from a cursor fetch operation. Provides both tuple-like indexing
and attribute access to column values.
Column attribute access behavior depends on the global 'lowercase' setting:
- When enabled: Case-insensitive attribute access
- When disabled (default): Case-sensitive attribute access matching original column names
Example:
row = cursor.fetchone()
print(row[0]) # Access by index
print(row.column_name) # Access by column name (case sensitivity varies)
`
This documentation will help users understand the conditional behavior and avoid confusion when migrating between different lowercase settings.
Documentation Enhancement Needed: The new lowercase global variable should be documented in the package documentation to help users understand its behavior and impact. Recommendation: Add documentation for the lowercase global to the main package README. This should include: Purpose: Explain that lowercase controls column name case sensitivity in Row attribute access Default behavior: lowercase=False (case-sensitive, preserves original column names) When enabled: lowercase=True enables case-insensitive attribute access by converting all column names to lowercase Performance note: When enabled, adds slight overhead during cursor description initialization for column name processing Usage Example: ` Enable case-insensitive column accessmssql_python.lowercase = True Now all these work regardless of actual column casing:row.column_name # Works Add a new "Configuration Options" section to the main package README, or include it in an existing "Usage" section if one exists. This documentation will help users understand when and why they might want to enable this feature, especially those migrating from other database libraries that have different case sensitivity behaviors. |
@@ -1313,6 +1314,76 @@ def test_row_column_mapping(cursor, db_connection): | |||
cursor.execute("DROP TABLE #pytest_row_test") | |||
db_connection.commit() | |||
|
|||
def test_lowercase_attribute(cursor, db_connection): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want you to think of following test cases as well... May be for futures:
Test case for concurrent cursors with different lowercase values
This would be valuable future work if per-cursor settings are supported. Currently, the implementation uses a global lowercase setting that affects all cursors created after the setting is changed. A test for this scenario could look like:
def test_concurrent_cursors_different_lowercase_settings(): """Test behavior when multiple cursors exist with different lowercase settings""" # This test would be relevant if per-cursor lowercase settings are implemented # Currently, the global setting affects all new cursors uniformly pass # Future implementation
Test for lowercase setting changes after cursor creation
The lowercase setting is read at cursor creation time during _initialize_description()
Changing mssql_python.lowercase after a cursor is created but before fetch should not affect that cursor's behavior
The cursor's column name casing is determined when the description is initialized
Here's a suggested test case:
`
def test_lowercase_setting_after_cursor_creation(cursor, db_connection):
"""Test that changing lowercase setting after cursor creation doesn't affect existing cursor"""
original_lowercase = mssql_python.lowercase
try:
# Create table and cursor with lowercase=False
mssql_python.lowercase = False
cursor.execute("CREATE TABLE #test (UserName VARCHAR(50))")
cursor.execute("SELECT * FROM #test")
# Change setting after cursor is created but before fetch
mssql_python.lowercase = True
# The cursor should still use the original casing
column_names = [desc[0] for desc in cursor.description]
assert "UserName" in column_names, "Column casing should not change after cursor creation"
assert "username" not in column_names, "Lowercase should not apply to existing cursor"
finally:
mssql_python.lowercase = original_lowercase
`
The lowercase setting affects cursors at creation time, not at fetch time. Once a cursor's description is initialized, changing the global setting will not affect that cursor's column name casing.
This behavior should be clearly documented in the docstrings and potentially in user-facing documentation to avoid confusion.
def get_settings(): | ||
return _settings | ||
|
||
lowercase = _settings.lowercase # Default is False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation of the global lowercase setting introduces potential race conditions in multi-threaded environments. Here are the key concerns:
Current Issue
The global lowercase variable is mutable state that can be modified at runtime:
`
This creates a race condition in multi-threaded code
mssql_python.lowercase = True # Thread A
mssql_python.lowercase = False # Thread B (concurrent modification)
`
Potential Problems
Race conditions: Multiple threads modifying lowercase simultaneously
Inconsistent behavior: Cursors created in different threads may get different column casing unexpectedly
Thread interference: One thread's setting change affects all other threads globally
This I have mentioned in my other comments as well.. But I wanted to enforce this here as well.
If the current global approach is maintained, the documentation should clearly state:
-
Not thread-safe: Concurrent modifications to mssql_python.lowercase can cause undefined behavior
-
Recommended usage: Set once at application startup before creating any connections/cursors
-
Multi-threading: Use connection-level or cursor-level overrides for thread-safe operation
This is particularly important given that threadsafety = 1 indicates "Threads may share the module, but not connections" in the DB-API specification, yet the global setting affects all connections across all threads.
Work Item / Issue Reference
Summary
This pull request introduces a new global
lowercase
flag to themssql_python
package, allowing users to control whether column names in query results are automatically converted to lowercase. The changes include updates to the global settings infrastructure, theCursor
andRow
classes to support case-insensitive access, and comprehensive tests to ensure correct behavior.Global lowercase flag and settings:
lowercase
flag and supporting infrastructure tomssql_python/__init__.py
, including aSettings
class and aget_settings()
accessor. The default value isFalse
.lowercase
flag for external use and updated tests to check its default value.Cursor and Row class enhancements:
Cursor
class to read the globallowercase
setting and pass it toRow
objects. The_initialize_description
method now lowercases column names in the description if the flag is set.Row
class to support attribute access by column name, respecting thelowercase
flag for case-insensitive lookup.Cursor
instance toRow
objects for correct lowercase handling.Testing improvements:
lowercase
flag intests/test_004_cursor.py
, verifying that column names in the cursor description and row attribute access are correctly lowercased when the flag is enabled.