Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-131507: Clean up tests and type checking for_pyrepl#131509
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
Looks good in general, minor nit in docstring
| The prompt string has the zero-width brackets recognized by shells | ||
| (\x01 and \x02) removed. The length ignores anything between those | ||
| brackets as well as any ANSI escape sequences. |
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.
| Thepromptstringhasthezero-widthbracketsrecognizedbyshells | |
| (\x01and \x02)removed.Thelengthignoresanythingbetweenthose | |
| bracketsaswellasanyANSIescapesequences. | |
| Thepromptstringhasthezero-widthbrackets(\x01and \x02) | |
| recognizedbyshellsremoved.Thelengthignoresanythingbetween | |
| thosebracketsaswellasanyANSIescapesequences. |
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.
Is this an improvement? There are many kinds of zero-width brackets, the ones I am interested in are those recognized by shells (\x01 and \x02). The original docstring uses that ordering then.
| return length - sum(len(i) for i in sequence) + ctrl_z_cnt | ||
| def unbracket(s: str, including_content: bool = False) -> str: |
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.
👍 I like this approach. I was concerned about the regular expressions performance but couldn't find anything too bad when testing
| def multiline_input(reader: ReadlineAlikeReader, namespace: dict | None = None): | ||
| saved = reader.more_lines | ||
| try: | ||
| reader.more_lines = partial(more_lines, namespace=namespace) | ||
| reader.ps1 = reader.ps2 = ">>>" | ||
| reader.ps3 = reader.ps4 = "..." | ||
| reader.ps1 = reader.ps2 = ">>> " |
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.
Why the extra space?
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.
Because that's how the real prompts are in production. I didn't want the difference in tests since it was confusing for syntax highlighting later.
| @@ -421,42 +421,15 @@ def calc_screen(self) -> list[str]: | |||
| @staticmethod | |||
| def process_prompt(prompt: str) -> tuple[str, int]: | |||
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.
I may be missing something but looks like the old function was tracking nested ANSI escape seas outside\x01 and\x02 brackets but this one drops this no? What is the rationale?
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.
There are three things the old function was trying to do (badly):
- ignore anything between \001 and \002 for length calculation; and
- remove \001 and \002 characters from the emitted prompt; and
- also ignore any ANSI escape sequences from length calculation.
Since wlen() is already doing 3., a contributor here needed to recreate this function to not do that, because we were doing that later anyway. But it's simpler to just use the main wlen() with a string stripped of \001 .. \002 bracketed content.
I find the new form much easier on the eyes.
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.
LGTM left some comments
5d8e981 intopython:mainUh oh!
There was an error while loading.Please reload this page.
Thanks@ambv for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13. |
Sorry,@ambv, I could not cleanly backport this to |
pythonGH-131509)(cherry picked from commit5d8e981)Co-authored-by: Łukasz Langa <lukasz@langa.pl>
GH-131546 is a backport of this pull request to the3.13 branch. |
…thonGH-131547)This is based offpythonGH-131509.(cherry picked from commit4cc82ff)Co-authored-by: Łukasz Langa <lukasz@langa.pl>
Uh oh!
There was an error while loading.Please reload this page.
This is a prerequisite for the syntax highlighting change, but a separate PR so that this can be backported to 3.13 for ease of maintenance in the future.
The separate commits explain the separate steps:
assert_screen_equalin more places_colorizebetter: enumerate all ANSI colors, make type checking passLib/_colorize.pyin the checks (via aMisc/mypy/trick with symlinks, let's see how that works out)