Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Conversation

@yihong0618
Copy link
Contributor

@yihong0618yihong0618 commentedSep 6, 2025
edited by bedevere-appbot
Loading

as diff and doc:
To quit this help utility and return to the interpreter,
enter "q", "quit" or "exit".

Signed-off-by: yihong0618 <zouzou0208@gmail.com>
Copy link
Member

@AA-TurnerAA-Turner left a 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

@bedevere-app
Copy link

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 phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@yihong0618
Copy link
ContributorAuthor

This needs a test. The wording of the NEWS entry should also be significantly improved, it's highly unclear at present.

copy that, will try to make it better thanks for the review

Signed-off-by: yihong0618 <zouzou0208@gmail.com>
@yihong0618
Copy link
ContributorAuthor

This needs a test. The wording of the NEWS entry should also be significantly improved, it's highly unclear at present.

A

Done sorry for my English...the better news help with LLM

@AA-Turner
Copy link
Member

Please be aware of our policy on use of LLMs and 'generative AI':https://devguide.python.org/getting-started/generative-ai/

@yihong0618
Copy link
ContributorAuthor

yihong0618 commentedSep 6, 2025
edited
Loading

Please be aware of our policy on use of LLMs and 'generative AI':https://devguide.python.org/getting-started/generative-ai/

thank you for let me know that.seems ok here, if not I will change to my own words
Thank you for the kindness again

Assistance with writing comments, especially in a non-native language

@yihong0618
Copy link
ContributorAuthor

changed it to my own

@ZeroIntensity
Copy link
Member

@adqm Would you care to review this, considering your recent work withhelp?

adqm reacted with thumbs up emojiyihong0618 reacted with heart emoji

Signed-off-by: yihong0618 <zouzou0208@gmail.com>
Copy link
Contributor

@adqmadqm left a 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.

…ZlalC.rstCo-authored-by: adam j hartz <hz@mit.edu>
@yihong0618
Copy link
ContributorAuthor

yihong0618 commentedSep 6, 2025
edited
Loading

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.

Thank you its very helpful but two things seem we need to disscuss

  1. do we need to handle empty string in this patch, since I wonder it is another issue and break change
  2. is it needs to add a helper function for two tests in unittest, the code is quite simple and right, but I am not sure it worth

Thanks again

Signed-off-by: yihong0618 <zouzou0208@gmail.com>Co-authored-by: adam j hartz <hz@mit.edu>
@yihong0618
Copy link
ContributorAuthor

@adqm Hi the tests code changes but the strip one seems not right in my env

Copy link
Contributor

@adqmadqm left a 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.

@yihong0618
Copy link
ContributorAuthor

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.

in my side “ ” in help mode
with or without this patch are the same
it shows the str help

@adqm
Copy link
Contributor

adqm commentedSep 7, 2025
edited
Loading

Yes, if you wrap it in quotes you will get thestr help. But if you just type some number of spaces with no quotation marks it should be ignored and return you to the prompt. Same goes for length-0 strings; with quotes the input isn't ignored (we see the usage suggestions), but without quotes (i.e., just hitting Enter at the prompt) it just returns you to the prompt.

That feels like the expected behavior to me. I think the handling of" " (with the quotation marks) is surprising, but changing that does feel like a separate issue.

@yihong0618
Copy link
ContributorAuthor

Yes, if you wrap it in quotes you will get thestr help. But if you just type some number of spaces with no quotation marks it should be ignored and return you to the prompt. Same goes for length-0 strings; with quotes the input isn't ignored (we see the usage suggestions), but without quotes (i.e., just hitting Enter at the prompt) it just returns you to the prompt.

That feels like the expected behavior to me. I think the handling of" " (with the quotation marks) is surprising, but changing that does feel like a separate issue.

Get it you are right will change it thank you very much

Co-authored-by: adam j hartz <hz@mit.edu>
Copy link
Contributor

@adqmadqm left a 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!

yihong0618 reacted with heart emoji
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
Copy link
Member

@AA-TurnerAA-Turner left a 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

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
@yihong0618
Copy link
ContributorAuthor

yihong0618 commentedSep 10, 2025
edited
Loading

Two minor suggestions, OK otherwise.

A

checked its better thanks the hint


will fix the test

Signed-off-by: yihong0618 <zouzou0208@gmail.com>
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
Copy link
Contributor

@adqmadqm left a 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.

yihong0618 reacted with thumbs up emoji
@yihong0618
Copy link
ContributorAuthor

One more thought about the updated test cases.

of course its better will commit and check

Co-authored-by: adam j hartz <hz@mit.edu>
Copy link
Contributor

@adqmadqm left a 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.

Comment on lines +2064 to 2067
if not request or request.isspace():
continue # back to the prompt
request = request.strip()

Copy link
Contributor

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.

Suggested change
ifnotrequestorrequest.isspace():
continue# back to the prompt
request=request.strip()
request=request.strip()
ifnotrequest:
continue# back to the prompt

ambv reacted with thumbs up emoji
@yihong0618yihong0618 closed this by deleting the head repositoryDec 16, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@AA-TurnerAA-TurnerAwaiting requested review from AA-TurnerAA-Turner is a code owner

@adqmadqmAwaiting requested review from adqm

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@yihong0618@AA-Turner@ZeroIntensity@adqm

[8]ページ先頭

©2009-2026 Movatter.jp