Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-133390: Support basic completion for sqlite3 command-line interface#133393
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 ourterms of service andprivacy 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
python-cla-botbot commentedMay 4, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
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.
This needs a whatsnew entry, (maybe a note in the docs?), a NEWS entry and tests.
This is quite limited completion so I guess it could work.
Uh oh!
There was an error while loading.Please reload this page.
I think we need a separate nodule for this because if we want to have a smarter completion later the code is better to be isolated, so something like sqlite3._completer. If possible, can we have anautogenerated list of keywords? and those keywords could also be stored in a (private) module-level list. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Misc/NEWS.d/next/Library/2025-05-05-03-14-08.gh-issue-133390.AuTggn.rst OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Please add tests |
Yes, tests is coming. I will try to find a way to autogenerate list of keywords. |
You can mark the pr as a draft. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
782a599
to5005d85
Comparetanloong commentedMay 5, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Added tests and moved keyword list to module level. Keywords seems not easy to auto-generate, theSQLite document says they can be accessed by |
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.
With#133447 in mind the keywords list would be quite handy (I think hardcoding is fine, it doesn't change often anyway). Maybe it could be moved elsewhere, what do you think Benedikt?
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
tanloong commentedMay 5, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hi@picnixz, as Stan mentioned, beta freeze is around the corner, but I really hope this can be shipped in 3.14, do you think there is a chance to merge it before beta freeze? |
@requires_subprocess() | ||
classCompletionTest(unittest.TestCase): | ||
PS1="sqlite> " |
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.
This is no longer freely customizable by users via sys.ps1 ?
Thank you so much for your review. Really appreciate your guidance! |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
tanloong commentedMay 16, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The Unfortunately I didn't find a Can we remove this |
picnixz commentedMay 16, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Let's just check that the completion of nothing returns all keywords. You can sort the |
GNU Readline always sorts completions before displaying them. There is a [rl_sort_completion_matches](https://git.savannah.gnu.org/cgit/readline.git/tree/complete.c#n411) in Readline's source code but it's not exposed as a config flag.
Modules/_sqlite/module.c Outdated
gotoerror; | ||
} | ||
} | ||
if (PyModule_Add(module,"SQLITE_KEYWORDS",keywords)<0) { |
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.
PyModule_Add()
"eats" keywords, soPy_XDECREF
should not be called after error. Just return -1. Or even return the result ofPyModule_Add()
.
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.
cc@encukou who implemented this I believe.
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.
May I try to update the C code. As this PR is sent from my fork, it would be more convenient that I make the change. I will convert if I mess it up.
Uh oh!
There was an error while loading.Please reload this page.
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.
LGTM.
bedevere-bot commentedMay 29, 2025
🤖 New build scheduled with the buildbot fleet by@encukou for commit8d4f659 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F133393%2Fmerge If you want to schedule another build, you need to add the🔨 test-with-buildbots label again. |
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.
Thank you! This looks great.
Let's check the buildbots before merging, as readline can be quirky. (So can the buildbots: not are stable so don't be alarmed if there are failures.)
There are failures on FreeBSD, and onUbuntu and someFedora/RHEL configurations :( |
Yes, I will look into it. |
Disable coloring in main process to see if it resolves buildbots failures
tanloong commentedMay 31, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
FreeBSD: When using It is mentioned in other issues as well that the Ubuntu and Fedora/RHEL: Could not reproduce on my virtual machines. I suspect that the The subprocess created by |
Uh oh!
There was an error while loading.Please reload this page.
This adds tab-completion for the147 SQLite Keywords. A whitespace is appended to completion candidates for users' convenience and to mimic the behavior ofsqlite3 tool.