GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-133934: Improve `.help` in the `sqlite3` CLI by tanloong · Pull Request #133935 · python/cpython

Conversation

tanloong commented May 12, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Make .help in the sqlite3 CLI 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 .help output looks like this:

Without arguments, list available commands

sqlite> .help 
Documented commands (type .help <command>):
===========================================
help  q  quit  version

With one command name, print help for that command

sqlite> .help help
        Usage: .help [-all] [command]
        
        Without argument, print the list of available commands.
        With a command name as argument, print help about that command.
        With more command names as arguments, only the first one is used.
sqlite> .help unknown
No help for 'unknown'

With -all or --all, print help for all commands

sqlite> .help -all
.help [-all] [command]
        Without argument, print the list of available commands.
        With a command name as argument, print help about that command.
        With more command names as arguments, only the first one is used.
.q(uit)
        Exit this program.
.version
        Show version of the runtime SQLite library.

If passed with two or more arguments, only use the first one

sqlite> .help version unknown
        Usage: .version
        
        Show version of the runtime SQLite library.
sqlite> .help unknown version
No help for 'unknown'

Issue: Improve .help in the sqlite3 CLI #133934

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

tanloong added 2 commits

May 12, 2025 23:03


          Improve .help in sqlite3 CLI

8500c98

Make `.help` print list of available commands and `.help <command>` prints help for that command


          Add news entry

b28f24f

bedevere-app Bot mentioned this pull request

May 12, 2025

Improve .help in the sqlite3 CLI #133934

Closed


          Fix news entry typo

8dab230

tanloong marked this pull request as ready for review

May 12, 2025 15:09

tanloong requested review from berkerpeksag and erlend-aasland as code owners

May 12, 2025 15:09

bedevere-app Bot added the awaiting review label

May 12, 2025

StanFromIreland requested changes

May 12, 2025

View reviewed changes

StanFromIreland left a comment

Copy link

Copy Markdown

Member

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 use print( ... , file=sys.XXX) and not self.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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-app Bot added awaiting core review and removed awaiting review labels

May 12, 2025

tanloong commented May 12, 2025

Copy link

Copy Markdown

Contributor Author

Hi @StanFromIreland , thank you for your review!

use print( ... , file=sys.XXX)

Good point! I agree and will update the code.

You have made a mess with the color logic, with "helper" functions, some functions now using it and some not.

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.

There is also a lot of duplication now with unnecessary PS1/2 definitions etc.

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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

StanFromIreland reviewed

May 12, 2025

View reviewed changes

Comment thread

Lib/sqlite3/__main__.py

Comment on lines +44 to +45

		PS1 = "sqlite> "
		PS2 = " ... "

StanFromIreland May 12, 2025

Copy link

Copy Markdown

Member

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 use sys.ps1 like before, this would break current behavior.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

tanloong May 25, 2025

Copy link

Copy Markdown

Contributor Author

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 self.PS1 is needed by this line to get the length of plain prompt string. The length can't be obtained through sys.ps1 because sys.ps1 has surrounding control sequences for coloring.

I tested it and it doesn't break current behavior.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/sqlite3/__main__.py

		def do_version(self, _):
		""".version

		Show version of the runtime SQLite library.

StanFromIreland May 12, 2025

Copy link

Copy Markdown

Member

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.

Suggested change

	Show version of the runtime SQLite library.
	Print version of the runtime SQLite library.

This is the more common way of describing this. And also this does not need a new function.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

tanloong May 25, 2025

Copy link

Copy Markdown

Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/sqlite3/__main__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/sqlite3/__main__.py

		self._print_commands(self.undoc_header, cmds_undoc, 80)
		else:
		arg = arg.split()[0]
		if arg in ("-all", "--all"):

StanFromIreland May 12, 2025

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/sqlite3/__main__.py

		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}'")

StanFromIreland May 12, 2025

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/sqlite3/__main__.py

Comment on lines +99 to +104

		def do_quit(self, _):
		""".q(uit)

		Exit this program.
		"""
		sys.exit(0)

StanFromIreland May 12, 2025

Copy link

Copy Markdown

Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

tanloong May 25, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

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 a do_ prefix to mark it as a dot command. That way the quit command is accessible by .help.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/sqlite3/__main__.py

		do_q = do_quit

		def _help_message_from_doc(self, doc):
		# copied from Lib/pdb.py#L2544

StanFromIreland May 12, 2025

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/sqlite3/__main__.py

		self.write(f"{t.message}{msg}{t.reset}\n")

		def _print_commands(self, header, cmds, maxcol):
		# copied and modified from Lib/cmd.py#L351

StanFromIreland May 12, 2025

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/sqlite3/__main__.py

		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

StanFromIreland May 12, 2025

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Misc/NEWS.d/next/Library/2025-05-12-23-00-10.gh-issue-133934.6t6Gvp.rst

Comment on lines +1 to +2

		Make ``.help`` in the :mod:`sqlite3` command-line interface print list of
		available commands and ``.help <command>`` prints help for that command.

StanFromIreland May 12, 2025

Copy link

Copy Markdown

Member

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.

Suggested change

	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 print a list of
	available commands and ``.help <command>`` print help for that command.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka commented May 12, 2025

Copy link

Copy Markdown

Member

I have a feeling you overdid it. We do not want to implement an exact replica of the sqlite3 program. It only implements three commands, and none of them are absolutely necessary (you can quit by pressing Ctrl-D or Ctrl-Z). They are fine when the implementation is one-line.

But the final decision is up to @erlend-aasland.

StanFromIreland reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

StanFromIreland commented May 12, 2025

Copy link

Copy Markdown

Member

I have a feeling you overdid it.

+1, IMO a reasonable approach would be non-intrusive and have a much lower diff.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland commented May 12, 2025

Copy link

Copy Markdown

Contributor

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.

StanFromIreland reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tanloong commented May 25, 2025

Copy link

Copy Markdown

Contributor Author

Feeling down on this in past few days. I want to explain that the goal of this PR isn't to replicate sqlite3's .help. When I said “this matches the behavior of the SQLite CLI tool,” I meant to show that the .help design in this PR is reasonable. I was worried that the PR might get rejected because people thought the .help design was self-opinioned. The goal is to make .help more helpful, we may have more dot commands in the future and need to list them to users at that time.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka reviewed

May 25, 2025

View reviewed changes

serhiy-storchaka left a comment

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

StanFromIreland commented Jun 23, 2025

Copy link

Copy Markdown

Member

Another approach to implementing this, #135224, has been merged.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tanloong closed this

Jun 23, 2025

Reviewers

serhiy-storchaka

serhiy-storchaka left review comments

picnixz

picnixz left review comments

StanFromIreland

StanFromIreland requested changes

berkerpeksag

Awaiting requested review from berkerpeksag

erlend-aasland

Awaiting requested review from erlend-aasland

erlend-aasland is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

tanloong commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StanFromIreland left a comment

Choose a reason for hiding this comment

Uh oh!

tanloong commented May 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanloong May 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented May 12, 2025

Uh oh!

StanFromIreland commented May 12, 2025

Uh oh!

erlend-aasland commented May 12, 2025

Uh oh!

tanloong commented May 25, 2025

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

StanFromIreland commented Jun 23, 2025

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tanloong commented May 12, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

tanloong May 25, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.