Skip to content
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.

Sign up for GitHub

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

Jump to bottom

bpo-35208: Fix IDLE Squeezer line counting #10449

Merged
merged 10 commits into from
Dec 24, 2018
Merged

bpo-35208: Fix IDLE Squeezer line counting #10449

merged 10 commits into from
Dec 24, 2018

Conversation

taleinat
Copy link
Contributor

@taleinat taleinat commented Nov 10, 2018 •
edited by bedevere-bot

This also fixes the label when squeezing a single line to "Squeezed text (1 line)", rather than "Squeezed text (1 lines)".

https://bugs.python.org/issue35208

@taleinat taleinat added type-bug An unexpected behavior, bug, or error needs backport to 3.6 skip news labels Nov 10, 2018
@taleinat taleinat force-pushed the bpo-35208/squeezer_line_counting_fix branch from 73e0fea to 675b42f Compare November 10, 2018 21:19
@taleinat taleinat force-pushed the bpo-35208/squeezer_line_counting_fix branch from 675b42f to 01fa746 Compare November 10, 2018 21:45
@taleinat
Copy link
Contributor Author

@terryjreedy, this is a bugfix and I really think it should go into 3.6 before it's too late. Would you like to review this or can I just merge it?

@terryjreedy
Copy link
Member

It is semi too late in that the rcs are already out. We would have to commit to 3.8 and 3.7.2+ and request cherry-pick to the two release branches. However, the plural issue is not critical, and as I said on issue, I don't necessarily consider reporting logical lines to be a bug. If I have misunderstood the issue, please clarify.

@terryjreedy
Copy link
Member

Example: >>> print(200*(70*'a'+'\n'))
Squeezed text 200 lines. Unsqueeze, narrow window till wrap. Squeeze. still 200 (logical) lines. This is ok with me, though I might add something to doc. If we do decide to change, it does not strike me now as a release blocker.

@terryjreedy
Copy link
Member

terryjreedy commented Dec 21, 2018 •
edited

At the moment, I cannot make any changes, or even make an inline comment. Same as for #11214.
Edit: fixed by switching to MS Edge.

terryjreedy
terryjreedy approved these changes Dec 23, 2018
View reviewed changes
Copy link
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

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

I added news items and made a couple of changes. If you are happy with them, go ahead and merge.

taleinat
taleinat commented Dec 24, 2018
View reviewed changes
Lib/idlelib/squeezer.py Outdated
# (1,0), even though a new line hadn't yet been started. The same
# is true if length is any exact multiple of linewidth. Therefore,
# subtract 1 before dividing a non-empty line.
if current_column > linewidth: # Don't bother adding 0.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change does avoid the current_column == 0 edge-case, but the way it is now written (including the comment) makes it seem like a questionable optimization rather than handling an important edge-case (which happens with empty lines).

I suggest at least expanding the comment, e.g.:

# Avoid the `current_column == 0` edge-case, and while we're at it,
# don't bother adding 0.

Copy link
Member

Choose a reason for hiding this comment

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

The comment is already longer than I would like, but I did not readily see how to nicely condense it. A bit more will not hurt.

@taleinat
Copy link
Contributor Author

@terryjreedy, thanks for the review and improvements!

I have a few comments, let me know what you think.

@taleinat taleinat merged commit 44a79cc into python:master Dec 24, 2018
@miss-islington
Copy link
Contributor

Thanks @taleinat for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-11305 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 24, 2018
@taleinat @miss-islington
bpo-35208: Fix IDLE Squeezer line counting (pythonGH-10449)
3165bac 
(cherry picked from commit 44a79cc)

Co-authored-by: Tal Einat <[email protected]>
miss-islington added a commit that referenced this pull request Dec 24, 2018
@miss-islington @taleinat
bpo-35208: Fix IDLE Squeezer line counting (GH-10449)
0e0cc55 
(cherry picked from commit 44a79cc)

Co-authored-by: Tal Einat <[email protected]>
@python python deleted a comment from bedevere-bot Dec 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terryjreedy terryjreedy terryjreedy approved these changes

Assignees
No one assigned
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@taleinat @terryjreedy @miss-islington @bedevere-bot @the-knights-who-say-ni