Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-133934: Improve.help in thesqlite3 CLI#133935
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
Make `.help` print list of available commands and `.help <command>` prints help for that command
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.
Please useprint( ... , file=sys.XXX) and notself.write, you must have not noticed the confusion it caused on your previous pr. You have made a mess with the color logic, with "helper" functions, some functions now using it and some not.
There is also a lot of duplication now with unnecessary PS1/2 definitions etc.
Hi@StanFromIreland , thank you for your review!
Good point! I agree and will update the code.
I'm not quite clear about which parts you find problematic. It would be very helpful if you could point out specific examples where the color logic is inconsistent or where the helper functions are misused or neglected. That way, I can address the issues precisely.
I have reviewed the current changes and didn't find any redundant definitions. If you could indicate the exact locations where you see this duplication, I'll take another close look and make improvements. Thank your again for your help! |
| PS1 = "sqlite> " | ||
| PS2 = " ... " |
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.
Remove these and usesys.ps1 like before, this would break current behavior.
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.
Theself.PS1 is needed bythis line to get the length of plain prompt string. The length can't be obtained throughsys.ps1 becausesys.ps1 has surrounding control sequences for coloring.
I tested it and it doesn't break current behavior.
| def do_version(self, _): | ||
| """.version | ||
| Show version of the runtime SQLite library. |
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.
| ShowversionoftheruntimeSQLitelibrary. | |
| PrintversionoftheruntimeSQLitelibrary. |
This is the more common way of describing this. And also this does not need a new function.
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.
Just as explained at#133935 (comment), we do need a function here.
Uh oh!
There was an error while loading.Please reload this page.
| self._print_commands(self.undoc_header, cmds_undoc, 80) | ||
| else: | ||
| arg = arg.split()[0] | ||
| if arg in ("-all", "--all"): |
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.
We should not have both. Please only keep the-- option, we could have-a too but it is unnecessary IMO
| if (method := getattr(self, "do_" + arg, None)) is not None: | ||
| print(self._help_message_from_doc(method.__doc__)) | ||
| else: | ||
| self._error(f"No help for '{arg}'") |
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.
Just use print and add the theme normally IMO
| def do_quit(self, _): | ||
| """.q(uit) | ||
| Exit this program. | ||
| """ | ||
| sys.exit(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.
Not needed?
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.
Although it's just a one-liner we still need to put it in a function with ado_ prefix to mark it as a dot command. That way thequit command is accessible by.help.
| do_q = do_quit | ||
| def _help_message_from_doc(self, doc): | ||
| # copied from Lib/pdb.py#L2544 |
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 will change, we don't need it anyway for inter-stdlib copies
| self.write(f"{t.message}{msg}{t.reset}\n") | ||
| def _print_commands(self, header, cmds, maxcol): | ||
| # copied and modified from Lib/cmd.py#L351 |
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.
ditto
| Each column is only as wide as necessary. | ||
| Columns are separated by two spaces (one was not legible enough). | ||
| """ | ||
| # copied and modified from Lib/cmd.py#L359 |
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.
ditto
| Make ``.help`` in the :mod:`sqlite3` command-line interface print list of | ||
| available commands and ``.help <command>`` prints help for that command. |
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.
| Make ``.help`` in the:mod:`sqlite3` command-line interface print list of | |
| available commands and ``.help <command>``prints help for that command. | |
| Make ``.help`` in the:mod:`sqlite3` command-line interface printalist of | |
| available commands and ``.help <command>``print help for that command. |
I have a feeling you overdid it. We do not want to implement an exact replica of the But the final decision is up to@erlend-aasland. |
+1, IMO a reasonable approach would be non-intrusive and have a much lower diff. |
I did not have a chance to look at this PR yet, but I'll add that we do not want to replicate the SQLite shell. When we initially added the sqlite3 CLI, we had in mind that the code would be easy to read and well commented, so it could be used as a kind of tutorial for how to use the interactive console class. |
Feeling down on this in past few days. I want to explain that the goal of this PR isn't to replicate |
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.
Currently we have just 3 simple commands (none of them are strictly necessary). Their implementation takes like 13 lines. This PR doubles the size of__main__.py. It improves.help, but it was not necessary, and it spends too much code for this.
If we had 20 commands, with arguments, then this would be the right way.
Uh oh!
There was an error while loading.Please reload this page.
Make
.helpin thesqlite3CLI show a list of available commands, and.help <command>display help for that command. This matches the behavior of the SQLite CLI tool, though the output layout may not be exactly the same.The new
.helpoutput looks like this:-allor--all, print help for all commands.helpin thesqlite3CLI #133934