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
|
See also @berkerpeksag PR which logs readline version in the regrtest header: PR #2618. |
| def setUpModule(): | ||
| if verbose and hasattr(readline, "_READLINE_VERSION"): | ||
| print(f"readline version: {readline._READLINE_VERSION:#x}") | ||
| print(f"readline runtime version: {readline._READLINE_RUNTIME_VERSION:#x}") |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
We add these with:
PyModule_AddIntConstant(m, "_READLINE_VERSION", RL_READLINE_VERSION);
So maybe we don't need the hasattr check?
Also, we need to know this is libedit, and looking at the tests, we need:
is_editline = readline.__doc__ and "libedit" in readline.__doc__
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
So maybe we don't need the hasattr check?
I like the idea of using test_readline unchanged on Python implementations other than CPython. Tests cannot rely on private attributes in tests, not without @cpython_only or hasattr(). I added a comment to explain the rationale.
is_editline
Right, I also logged this flag, even if the other attributes are not available.
I chose to also expose rl_library_version as a private attribute to be able to log it in test_readline. It may help to debug subtle differences between two readline versions, and validate manually is_editline flag.
d2d8816
to
e193abc
Compare
| print(f"readline runtime version: {readline._READLINE_RUNTIME_VERSION:#x}") | ||
| if hasattr(readline, "_rl_library_version"): | ||
| print(f"readline library version: {readline._rl_library_version!r}") | ||
| print(f"is libedit? {is_editline}") |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@berkerpeksag version showing "readline implementation: libedit|GNU readline" can be little nicer.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I changed that.
| @@ -1427,6 +1427,7 @@ PyInit_readline(void) | |||
|
|
|||
| PyModule_AddIntConstant(m, "_READLINE_VERSION", RL_READLINE_VERSION); | |||
| PyModule_AddIntConstant(m, "_READLINE_RUNTIME_VERSION", rl_readline_version); | |||
| PyModule_AddStringConstant(m, "_rl_library_version", rl_library_version); | |||
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Why not uppercase like the other constants?
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't notice that the variable on the previous line is in lower case but exported as UPPER case. I converted my new constant to UPPER case and rename "rl" to READLINE, as done for the 2nd constant.
* test_readline logs the versions of libreadline when run in verbose mode * Add also readline._READLINE_LIBRARY_VERSION
| if hasattr(readline, "_READLINE_LIBRARY_VERSION"): | ||
| is_editline = ("EditLine wrapper" in readline._READLINE_LIBRARY_VERSION) | ||
| else: | ||
| is_editline = (readline.__doc__ and "libedit" in readline.__doc__) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I don't like duplicating the horrible doc check, and adding a new incompatible check for the same thing.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I don't like duplicating the horrible doc check, and adding a new incompatible check for the same thing.
IMHO such enhancement would deserves its own PR ;-)
No description provided.