Skip to content

[3.12] gh-109151: Enable readline in the sqlite3 CLI (GH-109152) #110352

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

Merged
merged 1 commit into from
Oct 4, 2023

Conversation

miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Oct 4, 2023

(cherry picked from commit 254e30c)

Co-authored-by: Serhiy Storchaka storchaka@gmail.com

(cherry picked from commit 254e30c)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@serhiy-storchaka: This change will land in 3.12.1 bugfix, does it server a Changelog entry? I'm fine with no Changelog entry.

@erlend-aasland or @serhiy-storchaka: Feel free to merge the change.

@serhiy-storchaka
Copy link
Member

I do not see anything wrong in the Changelog entry.

@vstinner
Copy link
Member

vstinner commented Oct 4, 2023

Oh sorry, I missed the fact that you already added a Changelog entry :-)

@vstinner vstinner merged commit bc1fe35 into python:3.12 Oct 4, 2023
@miss-islington miss-islington deleted the backport-254e30c-3.12 branch October 4, 2023 13:48
@vstinner
Copy link
Member

vstinner commented Oct 4, 2023

Merged, thanks.

@Yhg1s
Copy link
Member

Yhg1s commented Oct 4, 2023

Hang on, this is a new feature. Why should it be included in 3.12.1?

@vstinner
Copy link
Member

vstinner commented Oct 4, 2023

Hang on, this is a new feature. Why should it be included in 3.12.1?

IMO it's minor enough to tolerate it after 3.12.0 release. There is no risk of regression: there is a try/exception, and try/except ImportError to import readline is common. Also, readline is really pleasant to use :-) Well, that's my rationale.

@serhiy-storchaka
Copy link
Member

It is a borderline between a new feature and a bugfix. It is not the first time when a new feature (the sqlite CLI in this case) was polished after release.

@Yhg1s
Copy link
Member

Yhg1s commented Oct 5, 2023

It's really not borderline in my eyes. It's clearly a new feature and one that can be problematic, since importing readline can produce more than just ImportError (especially since it's readline, which wraps a C library of somewhat questionable predictability). Is the lack of readline a problem? How big of a problem? Why wasn't it caught before beta 1?

@serhiy-storchaka
Copy link
Member

It is not critical problem, but it makes the sqlite3 REPL less useful. Try to use Python REPL without readline, it always annoyed me that Python 2.7 did not import it by default. It is not a big problem to me on Linux, because I always use an external readline wrapper, but I think that other users would see the sqlite3 REPL as half-baked without readline support.

It wasn't caught before beta 1 because before #109108 I did not know that the CLIs of some of other modules support readline (unlike the raw InteractiveConsole) and that it is so easy to enable it.

@vstinner
Copy link
Member

vstinner commented Oct 6, 2023

Is the lack of readline a problem? How big of a problem?

It's mostly a usability issue.

Try Python REPL without readline to have an idea. Example when I try to get back with LEFT key to fix a typo:

vstinner@mona$ find -name "readline*.so" -delete
vstinner@mona$ ./python
Python 3.13.0a0 (heads/test_pipesize_default:f08aa06d4f, Oct  6 2023, 12:24:11) [GCC 13.2.1 20230728 (Red Hat 13.2.1-1)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> 1++2+3+4+5+6+^[[D^[[D

When I try to navigate in the history with UP key:

vstinner@mona$ ./python
Python 3.13.0a0 (heads/test_pipesize_default:f08aa06d4f, Oct  6 2023, 12:24:11) [GCC 13.2.1 20230728 (Red Hat 13.2.1-1)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> 1+2
3
>>> ^[[A

Well, the Python REPL remains usable, but honestly, it's way less pleasant to use.

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Oct 6, 2023

It's mostly a usability issue.

Yes. IMO it is a usability bug, which is why I think it is fine to backport it.

@Yhg1s
Copy link
Member

Yhg1s commented Oct 9, 2023

I'm sorry, but no. This is a new feature, and shouldn't have been backported to 3.12. I'm fully aware of the consequence of not having readline (I've used interactive Python on more than a few systems without readline support over the years), but importing readline has more consequences than just "cursor keys start working". If the missing readline was a big usability problem, it should have been caught long before rc1, let alone the final release.

It can easily break things, and this affects the whole Python release: if any change to 3.12.1 is risky, the entirely 3.12.1 upgrade becomes risky. We make backward compatibility guarantees for that very reason: we need people to be comfortable upgrading Python to new patch versions, so we have to avoid risks like these.

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Oct 9, 2023
@serhiy-storchaka
Copy link
Member

I am sorry. I did not think that this change can be controversial. I'll revert it.

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Oct 9, 2023
Yhg1s pushed a commit that referenced this pull request Oct 9, 2023
Revert "[3.12] gh-109151: Enable readline in the sqlite3 CLI (GH-109152) (#110352)"

This reverts commit bc1fe35.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants