Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork34k
gh-138568: make it the same behavior in repl help mode#138569
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
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
AA-Turner left a comment
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.
This needs a test. The wording of the NEWS entry should also be significantly improved, it's highly unclear at present.
A
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
yihong0618 commentedSep 6, 2025
copy that, will try to make it better thanks for the review |
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
yihong0618 commentedSep 6, 2025
Done sorry for my English...the better news help with LLM |
AA-Turner commentedSep 6, 2025
Please be aware of our policy on use of LLMs and 'generative AI':https://devguide.python.org/getting-started/generative-ai/ |
yihong0618 commentedSep 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
thank you for let me know that.seems ok here, if not I will change to my own words
|
yihong0618 commentedSep 6, 2025
changed it to my own |
ZeroIntensity commentedSep 6, 2025
@adqm Would you care to review this, considering your recent work with |
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
adqm left a comment
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.
Thanks@yihong0618! I think this is indeed an improvement to the interactivehelp prompt.
I have a few suggestions, though (one behavioral change, a few stylistic things with the test cases, and a suggested rephrasing of the NEWS entry).
Of course,@ZeroIntensity and@AA-Turner should feel free to chime in if they disagree with any of my suggestions.
Uh oh!
There was an error while loading.Please reload this page.
Misc/NEWS.d/next/Library/2025-09-06-08-29-08.gh-issue-138568.iZlalC.rst OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
…ZlalC.rstCo-authored-by: adam j hartz <hz@mit.edu>
yihong0618 commentedSep 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thank you its very helpful but two things seem we need to disscuss
Thanks again |
Signed-off-by: yihong0618 <zouzou0208@gmail.com>Co-authored-by: adam j hartz <hz@mit.edu>
yihong0618 commentedSep 7, 2025
@adqm Hi the tests code changes but the strip one seems not right in my env |
adqm left a comment
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.
Can you clarify the way in which you think this is not working? For example, an input that you provided that gave different output from expected?
For me, it seems to work (I tested this before sending my first review). Both an empty input and an input consisting only of spaces result in just returning me to a new prompt, and asking for help about other things still gives me appropriate help output.
Uh oh!
There was an error while loading.Please reload this page.
yihong0618 commentedSep 7, 2025
in my side “ ” in help mode |
adqm commentedSep 7, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Yes, if you wrap it in quotes you will get the That feels like the expected behavior to me. I think the handling of |
yihong0618 commentedSep 7, 2025
Get it you are right will change it thank you very much |
Co-authored-by: adam j hartz <hz@mit.edu>
adqm left a comment
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. One more minor note from me, and then we'll wait and see what@AA-Turner thinks.
Thanks!
Uh oh!
There was an error while loading.Please reload this page.
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
AA-Turner left a comment
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.
Two minor suggestions, OK otherwise.
A
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
yihong0618 commentedSep 10, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
checked its better thanks the hint will fix the test |
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
adqm left a comment
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.
One more thought about the updated test cases.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
yihong0618 commentedSep 10, 2025
of course its better will commit and check |
Co-authored-by: adam j hartz <hz@mit.edu>
adqm left a comment
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 to me! Just one lingering thought.
| if not request or request.isspace(): | ||
| continue # back to the prompt | ||
| request = request.strip() | ||
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 still think the following is cleaner than having the extra.isspace call. I don't think we're actually saving a string allocation that way, either, since in the case where we would be continuing,.strip() would take us down to the empty string, which is interned.
But this is a judgement call, and it's ultimately up to@AA-Turner.
| ifnotrequestorrequest.isspace(): | |
| continue# back to the prompt | |
| request=request.strip() | |
| request=request.strip() | |
| ifnotrequest: | |
| continue# back to the prompt |
Uh oh!
There was an error while loading.Please reload this page.
as diff and doc:
To quit this help utility and return to the interpreter,
enter "q", "quit" or "exit".