Skip to content

bpo-35208: Fix IDLE Squeezer line counting #10449

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
Merged
taleinat merged 10 commits into from
Dec 24, 2018
Merged

bpo-35208: Fix IDLE Squeezer line counting #10449

taleinat merged 10 commits into from
Dec 24, 2018

Conversation

@taleinat
Copy link
Contributor

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

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 requested a review from terryjreedy November 10, 2018 07:34
@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

taleinat commented Dec 12, 2018

@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

terryjreedy commented Dec 12, 2018

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

terryjreedy commented Dec 12, 2018

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
Loading

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

@taleinat taleinat Dec 24, 2018

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

@terryjreedy terryjreedy Dec 24, 2018

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
taleinat commented Dec 24, 2018
View reviewed changes
taleinat
taleinat commented Dec 24, 2018
View reviewed changes
@taleinat
Copy link
Contributor Author

taleinat commented Dec 24, 2018

@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

miss-islington commented Dec 24, 2018

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

@bedevere-bot
Copy link

bedevere-bot commented Dec 24, 2018

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 <taleinat+github@gmail.com>
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 <taleinat+github@gmail.com>
@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.

5 participants

@taleinat @terryjreedy @miss-islington @bedevere-bot @the-knights-who-say-ni