Skip to content
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

gh-112510: Add readline.backend for the backend readline uses #112511

Merged
merged 7 commits into from
Dec 1, 2023

Conversation

gaogaotiantian
Copy link
Member

@gaogaotiantian gaogaotiantian commented Nov 29, 2023

@gaogaotiantian
Copy link
Member Author

@encukou @corona10 could you take a look? Thanks!

Modules/readline.c Outdated Show resolved Hide resolved
@@ -3293,7 +3293,7 @@ def setUpClass():
# Ensure that the readline module is loaded
# If this fails, the test is skipped because SkipTest will be raised
readline = import_module('readline')
if readline.__doc__ and "libedit" in readline.__doc__:
if readline.backend == "editline":
Copy link
Member

Choose a reason for hiding this comment

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

For just internal usage, why not just use readline._backend?

Co-authored-by: Donghee Na <[email protected]>
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

LGTM from my view. But I will leave it to @encukou since we have to export backend as the public. Once we export it we should maintain it for a long time and Petr is the expert in this area.

@encukou
Copy link
Member

encukou commented Nov 30, 2023

Thank you!
My main worry here is that we might get into a situation like sys.version/sys.version_info, where the earlier “stringly typed” attribute has a better name.
Are we likely to need something like backend.name & backend.version in the future? If we do, will it be OK to keep backend and a new backend_version?
Or am I overthinking this?


Could you add a test to assert backend is either "readline" or "editline"?

@gaogaotiantian
Copy link
Member Author

Thank you! My main worry here is that we might get into a situation like sys.version/sys.version_info, where the earlier “stringly typed” attribute has a better name. Are we likely to need something like backend.name & backend.version in the future? If we do, will it be OK to keep backend and a new backend_version? Or am I overthinking this?

Could you add a test to assert backend is either "readline" or "editline"?

Very helpful insight! From my observation, the difference between different versions of the same library is significantly less important than the difference between the two libraries. I can't guarantee that the version of the backend will not ever be used at all in the future, but I would guess that's a very rare case. Both readline and editline seems to be relatively consistent between versions.

That being said, I think using a single string type for readline.backend is reasonable, and it probably makes the life easier for the users compared to having a (name, version) tuple for the attribute and the users would have to do readline.backend[0] == 'readline' in most of the cases.

If in the future, readline or editline has a breaking change in some version that affects many users, having a readline.backend_version is not the end of the world.

@gaogaotiantian
Copy link
Member Author

I think we are done with the review for this PR? We should merge this before #107748 so it can use the backend instead of checking __doc__

encukou
encukou previously approved these changes Dec 1, 2023
@encukou encukou dismissed their stale review December 1, 2023 13:22

One more thing!

@encukou
Copy link
Member

encukou commented Dec 1, 2023

One more thing: the docs entry needs a versionadded. I updated the PR directly to avoid a round of review ping-pong; hope you don't mind.

(And I fell into the trap of thinking this is simple enough to use GitHub UI rather than a local clone...)

@encukou encukou enabled auto-merge (squash) December 1, 2023 13:40
@encukou encukou merged commit c298238 into python:main Dec 1, 2023
27 checks passed
@gaogaotiantian
Copy link
Member Author

Thanks for the quick review!

@gaogaotiantian gaogaotiantian deleted the readline-backend branch December 1, 2023 18:07
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
…pythonGH-112511)


Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
Co-authored-by: Donghee Na <[email protected]>
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
…pythonGH-112511)


Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
Co-authored-by: Donghee Na <[email protected]>
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.

3 participants