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-128066: Fixed PyREPL history saving on read-only file systems#128088

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

vladimir-poghosyan
Copy link

@vladimir-poghosyanvladimir-poghosyan commentedDec 19, 2024•
edited
Loading

As suggested in the comment of the linked issue by@ZeroIntensity, I simply handled the exception with a resulting warning.

@ghost
Copy link

ghost commentedDec 19, 2024•
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

Copy link
Member

@skirpichevskirpichev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM in general, with few concerns.

  1. Maybe it worth a test.
  2. I worry, that error is too late. Though, this will be fixed bygh-127495: Append to history file after every statement in PyREPL #132294.

@vladimir-poghosyan
Copy link
Author

Thank you for reviewing the PR.

I looked through your mentioned pull request an noticed that the call toappend_history_file is wrapped inexcept (FileNotFoundError, PermissionError) exception handler, thus still prone to the same read-only file system error, or am I missing something?

Will it be better, that I addOSError to theexcept (FileNotFoundError, PermissionError) in thesite.py (where thewrite_history_file is called) with a warning message and leave its implementation as it was? In that way file handling code in bothwrite_history_file andappend_history_file methods will match.

@skirpichev
Copy link
Member

I looked through your mentioned pull request an noticed that the call to append_history_file is wrapped in except (FileNotFoundError, PermissionError) exception handler

Now I also catchOSError like in your pr.

Will it be better, that I add OSError to the except (FileNotFoundError, PermissionError) in thesite.py

Hmm, maybe. (BTW I doubt thatFileNotFoundError is relevant at all.)

@vladimir-poghosyan
Copy link
Author

vladimir-poghosyan commentedApr 11, 2025•
edited
Loading

I've changed my implementation and moved error handling to thesite.py. Regarding the theFileNotFoundError being caught, I think that it is relevant for missing directories in the history file path.

skirpichev reacted with thumbs up emoji

@ambv
Copy link
Contributor

Closing in favor of#134380.

@ambvambv closed thisMay 20, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@skirpichevskirpichevskirpichev left review comments

@pablogsalpablogsalAwaiting requested review from pablogsal

@lysnikolaoulysnikolaouAwaiting requested review from lysnikolaou

@ambvambvAwaiting requested review from ambv

@FFY00FFY00Awaiting requested review from FFY00FFY00 is a code owner

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@vladimir-poghosyan@skirpichev@ambv
[8]ページ先頭
©2009-2025 Movatter.jp