-
Notifications
You must be signed in to change notification settings - Fork 824
Fixed an issue where the 'Quote strings only' configuration was ignored when downloading the result set. #7578 #9628
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: master
Are you sure you want to change the base?
Conversation
…ed when downloading the result set. pgadmin-org#7578
WalkthroughAdds a cursor-level numeric typecaster registration function and invokes it in the psycopg3 connection flow; updates a SQL editor CSV download test to include a numeric "Price" column and adjust assertions accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@web/pgadmin/tools/sqleditor/tests/test_download_csv_query_tool.py`:
- Line 32: The SQL string passed as the sql= parameter in
test_download_csv_query_tool.py exceeds 79 chars; break it into multiple shorter
string literals (e.g., wrap the argument in parentheses and use implicit
concatenation or split into two/three quoted pieces) so each line is under 79
characters while preserving the exact SQL content (including quotes around
column aliases and the numeric cast "2300::numeric as \"Price\""). Update the
sql= argument in the test function to use the multi-line string form so
pycodestyle E501 is resolved.
In `@web/pgadmin/utils/driver/psycopg3/typecast.py`:
- Around line 172-183: The continuation lines in register_numeric_typecasters
are over-indented causing pycodestyle E127; reformat the
_cursor.adapters.register_loader calls so continuation lines align under the
opening parenthesis instead of being visually over-indented—e.g., for each call
in register_numeric_typecasters (the calls that register NumericLoader,
FloatLoader, IntLoader) move the second-line indentation to match the position
after the opening parenthesis of _cursor.adapters.register_loader or place the
arguments on the same line to satisfy E127.
🧹 Nitpick comments (2)
web/pgadmin/tools/sqleditor/tests/test_download_csv_query_tool.py (1)
173-174: Consider also updating remainingassertTrue(... in ...)calls toassertIn.Lines 173-174 and 185-186 in the other test branches still use
self.assertTrue(self.output_columns in csv_data). For consistency with the changes on lines 208-209, these could be updated too.Also applies to: 185-186
web/pgadmin/utils/driver/psycopg3/typecast.py (1)
172-183: Consider adding OID 23 (int4) and OID 21 (int2) for consistency with bigint registration.OID 23 and OID 21 are not currently registered in
register_numeric_typecasters, nor are they listed inTO_STRING_NUMERIC_DATATYPES. They will use psycopg3's default loaders, which return native Python integers—so they work correctly out of the box. However, for consistency with OID 20 (bigint), which explicitly usesIntLoaderat the cursor level to override the globalTextLoaderpgAdminregistration, you may want to add similar registrations for int4 and int2.
Summary by CodeRabbit
Bug Fixes
Tests