-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
bpo-41373: IDLE: Fix saving files loaded with no newlines or mixed newlines #21597
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpo-41373: IDLE: Fix saving files loaded with no newlines or mixed newlines #21597
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.
I would like to make two changes other than the one in the comment below.
-
Line 140 'Will utf-8 decode'. Replace with encoding might be unknown rather than wrong.
-
UnicodeDecodeError on 152 can now only come from the 2nd read in 144-9.
Add to this PR? Separate PR for this issue? Separate issue?
Lib/idlelib/iomenu.py
Outdated
if not isinstance(eol_convention, str): | ||
# If the file does not contain line separators, it is None. | ||
# If the file contains mixed line separators, it is a tuple. | ||
eol_convention = os.linesep # default |
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.
line 124 above is identical, except it sets eol_convention as a class attribute. The class attribute is used in fixnewlines (the only usage) for new files created in IDLE. Rather than duplicate the default, I would rather only set the instance attribute when it might not be the default.
if isinstance(eol_convention, str):
self.eol_convention = eol_convention # Line 172 below.
else:
# If .... None.
# If ... tuple.
if eol_convention is not None:
# Message box as below.
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.
When you call loadfile()
for the same edit window second time, eol_convention
will be set to the EOL convention in previously opened file,
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.
At present, I don't think loadfile() is ever called more than once for an edit window*. Which means that the later line deleting current content is a no-op. But I would like to add a 'reload' function, such an when one want to restart editing, or a file is changed externally. So I agree with leaving loadfile as is, ready for such an addition.
*Except in the htest for iomenu, and I want that to continue to work.
However, the line changing eol_convention from None/tuple to string must be moved after the test for it being tuple (not None). As it is, editing a blank file brings up a spurious mixed endings message. I will change this and add a blurb.
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.
Good catch!
AFAIK loadfile() is called for the same edit window when you pick a recent file from menu.
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.
Recent files not already being edited are loaded in a new window, leaving current windows alone. Asking for a file already loaded switches to its window. What I would like to add is a popup offering to discard changes and reload.
When you're done making the requested changes, leave the comment: |
I did not fully understand you, but I think it is a separate issue. |
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
I will leave other changes to another issue after adding tests in a followup PR. I tested this change after the line was moved by loading and saving both a blank file and one with different endings, and reloading the result of the latter and checking that it only had default endings. I added blurb and will merge when CI done. |
Thanks @serhiy-storchaka for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9. |
…wlines (pythonGH-21597) Fixes regression in 3.8.4 and 3.9.0b4. Co-authored-by: Terry Jan Reedy <[email protected]> (cherry picked from commit 0dd463c) Co-authored-by: Serhiy Storchaka <[email protected]>
GH-21610 is a backport of this pull request to the 3.9 branch. |
GH-21611 is a backport of this pull request to the 3.8 branch. |
…wlines (pythonGH-21597) Fixes regression in 3.8.4 and 3.9.0b4. Co-authored-by: Terry Jan Reedy <[email protected]> (cherry picked from commit 0dd463c) Co-authored-by: Serhiy Storchaka <[email protected]>
…wlines (GH-21597) Fixes regression in 3.8.4 and 3.9.0b4. Co-authored-by: Terry Jan Reedy <[email protected]> (cherry picked from commit 0dd463c) Co-authored-by: Serhiy Storchaka <[email protected]>
…wlines (GH-21597) Fixes regression in 3.8.4 and 3.9.0b4. Co-authored-by: Terry Jan Reedy <[email protected]> (cherry picked from commit 0dd463c) Co-authored-by: Serhiy Storchaka <[email protected]>
Thank you for adding blurb Terry. |
…wlines (pythonGH-21597) Fixes regression in 3.8.4 and 3.9.0b4. Co-authored-by: Terry Jan Reedy <[email protected]>
…wlines (pythonGH-21597) Fixes regression in 3.8.4 and 3.9.0b4. Co-authored-by: Terry Jan Reedy <[email protected]>
…wlines (pythonGH-21597) Fixes regression in 3.8.4 and 3.9.0b4. Co-authored-by: Terry Jan Reedy <[email protected]>
https://bugs.python.org/issue41373