-
Notifications
You must be signed in to change notification settings - Fork 36
FEAT: Explicit parameters and improved type hints for bulkcopy #420
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
Conversation
…IDE discoverability - All options now explicit in function signature - Pass params directly to pycore (no kwargs dict conversion) - Matches mssql-tds explicit params API Based on community feedback from discussion #414
c90f0f1 to
fa2eab4
Compare
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
Refactors Cursor._bulkcopy to replace **kwargs bulk-copy options with an explicit, type-hinted parameter list and updated docstring.
Changes:
- Replaced
**kwargsin_bulkcopywith explicit parameters (e.g.,batch_size,timeout,column_mappings, and bulk-copy flags). - Expanded
_bulkcopydocstring to document supported bulk copy options. - Updated the
pycore_cursor.bulkcopy(...)invocation to pass the new explicit parameters.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. 📋 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.pybind.ddbc_bindings.cpp: 69.3%
mssql_python.pybind.ddbc_bindings.h: 69.7%
mssql_python.pybind.connection.connection.cpp: 75.3%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 79.6%
mssql_python.connection.py: 84.1%
mssql_python.cursor.py: 84.7%🔗 Quick Links
|
After Saurabh's review, Rust bulkcopy now uses direct types (bool, usize) instead of Option<T>. Python must not pass None values - instead omit the kwarg and let PyO3 use its defaults.
…/microsoft/mssql-python into bewithgaurav/fix-bulkcopy-kwargs
- batch_size: int = 0 (0 means server optimal) - timeout: int = 30 - column_mappings: Optional[Union[List[str], List[Tuple[int, str]]]] = None - All boolean flags: bool = False (not Optional[bool] = None) Updated docstring with two column_mappings formats: - Simple: List[str] - position = source index - Advanced: List[Tuple[int, str]] - explicit (index, column) mapping Simplified bulkcopy call to pass params directly (no kwargs dict) since Rust API now uses explicit defaults per Saurabh's review.
subrata-ms
left a comment
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.
approved with suggession
Work Item / Issue Reference
Summary
Implements Revision 2 of the BulkCopy API spec from community feedback in #414.
Replaces
**kwargswith explicit, type-hinted parameters for better IDE autocomplete and discoverabilityChanges
API Signature
What Changed
**kwargsOptional[bool] = Nonebool = Falsebatch_sizeOptional[int] = Noneint = 0(0 = server optimal)column_mappingsList[Tuple[Union[str, int], str]]Union[List[str], List[Tuple[int, str]]]Column Mappings
Per @amachanic's feedback, now supports simpler format:
Simple -
List[str]:Advanced -
List[Tuple[int, str]]: