bpo-29854: test_readline logs versions in verbose mode #2619
Conversation
|
See also @berkerpeksag PR which logs readline version in the regrtest header: PR #2618. |
Lib/test/test_readline.py
Outdated
| 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.
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.
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.
Lib/test/test_readline.py
Outdated
| 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.
@berkerpeksag version showing "readline implementation: libedit|GNU readline" can be little nicer.
Modules/readline.c
Outdated
| @@ -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.
Why not uppercase like the other constants?
There was a problem hiding this comment.
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.
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.
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.