Skip to content

Conversation

@csabella
Copy link
Contributor

@csabella csabella commented Jun 4, 2018

@terryjreedy
Copy link
Member

I expect to merge this tomorrow after I finish reviewing.

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 changed the yview arg to f'{n}.0'.

  1. I don't like passing floats as indexes, because A) they are not. They usually work because _tkinter calls str(float), but B) this is potentially fragile; for instance, float(3*.1) == 0.30000000000000004. We now C. have f-strings in addition to %-formatting as a way to create a string index. Both are D. 50% faster than str(float(n)). I prefer f-strings as the clearest way to make an index string.

  2. I like yview(int) even less than yview(float). A. Having yview(n+1) = yview(float(n)) is wretched. This made the tests confusing. B. the tk doc calls it obsolete. It could disappear from tkinter, as has happened with other obsolete tk features.

Otherwise, this is great. Thanks for the PR.

@terryjreedy terryjreedy changed the title bpo-33768: IDLE: clicking on context line moves it to top of editor bpo-33768: Clicking on IDLE context line moves it to top of editor Jun 8, 2018
@terryjreedy terryjreedy changed the title bpo-33768: Clicking on IDLE context line moves it to top of editor bpo-33768: IDLE: Clicking on code context line moves it to top of editor Jun 8, 2018
@terryjreedy terryjreedy added type-feature A feature request or enhancement needs backport to 3.6 labels Jun 8, 2018
@terryjreedy terryjreedy merged commit 041272b into python:master Jun 8, 2018
@miss-islington
Copy link
Contributor

Thanks @csabella for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-7515 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 Jun 8, 2018
…tor (pythonGH-7411)

(cherry picked from commit 041272b)

Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>
@bedevere-bot
Copy link

GH-7516 is a backport of this pull request to the 3.6 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 8, 2018
…tor (pythonGH-7411)

(cherry picked from commit 041272b)

Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>
miss-islington added a commit that referenced this pull request Jun 8, 2018
…tor (GH-7411)

(cherry picked from commit 041272b)

Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>
miss-islington added a commit that referenced this pull request Jun 8, 2018
…tor (GH-7411)

(cherry picked from commit 041272b)

Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>
@csabella
Copy link
Contributor Author

csabella commented Jun 8, 2018

I changed the yview arg to f'{n}.0'.
Thanks! I had copied this from the center method in editor.py and didn't even think about it, but your explanation makes sense.

@csabella csabella deleted the contextclick branch June 8, 2018 10:38
@terryjreedy
Copy link
Member

IDLE has had multiple authors, without a tkinter style guide ;-). I could see using the float trick when the alternative was % or .format, even though I did not like it, but then I realized that we have the new option since 3.6 that I like even better. So I did the timing test. While float alone is faster than f-string, _tkinter has to apply str, and the combination is slower., and I am fairly sure it still is even in C (where the lookup of 'str' is done at compile time).

There is also a matter of consistency. If row and column are known constants, such as 1 and 5 in the test, writing r.c (1.5) instead of 'r.c' ('1.5'), deferring string conversion to _tkinter, seems a bit silly to me, and I notice that you did not do that. A float() call is only useful for variable line, fixed column 0. The new f-strings always work when either line or column is a computed name rather than a literal or symbolic (end) constant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type-feature A feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants