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.

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-39152: add missing ttk.Scale.configure return value #17815

Merged
merged 3 commits into from Jan 5, 2020

Conversation

@terryjreedy
Copy link
Member

terryjreedy commented Jan 3, 2020

tkinter.ttk.Scale().configure([name]) now returns a configuration tuple for name
or a list thereof for all options. Based on patch Giovanni Lombardo.

https://bugs.python.org/issue39152

tkinter.ttk.Scale().configure() now returns a configuration.
Change posted by Giovanni Lombardo.
Copy link
Member

serhiy-storchaka left a comment

Please add a test.

@serhiy-storchaka

This comment has been minimized.

Copy link
Member

serhiy-storchaka commented Jan 4, 2020

Actually it would be enough to remove special cases for Scale in existing tests.

@terryjreedy

This comment has been minimized.

Copy link
Member Author

terryjreedy commented Jan 4, 2020

tkinter.test.widget.AbstractWidgetTest has two tests skip for Scale. I removed the skip in test_keys that works with this patch. The skipped part of test_param,

        if not isinstance(widget, Scale):
            t = widget.configure(name)
            self.assertEqual(len(t), 5)
            self.assertEqual2(t[4], expected, eq=eq)

still fails after the patch, 9 times, at the configure call because the override tries to update kw with a string.

Traceback (most recent call last):
  File "f:\dev\3x\lib\tkinter\test\widget_tests.py", line 497, in test_variable
    self.checkVariableParam(widget, 'variable', var)
  File "f:\dev\3x\lib\tkinter\test\widget_tests.py", line 195, in checkVariableParam
    self.checkParam(widget, name, var, conv=str)
  File "f:\dev\3x\lib\tkinter\test\widget_tests.py", line 67, in checkParam
    t = widget.configure(name)
  File "f:\dev\3x\lib\tkinter\ttk.py", line 1088, in configure
    kw.update(cnf)
ValueError: dictionary update sequence element #0 has length 1; 2 is required

The solution is to not do this. I will submit a further commit that fixes this and removes the 2nd skip.

Don't update dict with None or string.
Remove skip of param test that now works.
update blurb.
@terryjreedy

This comment has been minimized.

Copy link
Member Author

terryjreedy commented Jan 4, 2020

Scale.configure now returns the value returned by Widget.configure called with unaltered arguments. If tests pass, as they did on my machine, I think this is ready to merge.

The additional override code only involves tkinter's addition of '<>' events. Since Scale is not currently included in the ttk doc, the docstring is the only mention of the addition. (Since the notification results from action by the program getting the notice (rather than by a user), I am not sure why it is useful.)

@terryjreedy terryjreedy merged commit 5ea7bb2 into python:master Jan 5, 2020
9 checks passed
9 checks passed
Docs
Details
Windows (x86)
Details
Windows (x64)
Details
macOS
Details
Ubuntu
Details
Azure Pipelines PR #20200104.24 succeeded
Details
bedevere/issue-number Issue number 39152 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@terryjreedy terryjreedy deleted the terryjreedy:ttk-scale branch Jan 5, 2020
@miss-islington

This comment has been minimized.

Copy link

miss-islington commented Jan 5, 2020

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

@miss-islington

This comment has been minimized.

Copy link

miss-islington commented Jan 5, 2020

Sorry @terryjreedy, I had trouble checking out the 3.8 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 5ea7bb25e3b192d6c49a49c9e3b316f8559602aa 3.8

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Jan 5, 2020

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

miss-islington added a commit to miss-islington/cpython that referenced this pull request Jan 5, 2020
tkinter.ttk.Scale().configure([name]) now returns a configuration tuple for name
or a list thereof for all options. Based on patch Giovanni Lombardo.
(cherry picked from commit 5ea7bb2)

Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
@miss-islington

This comment has been minimized.

Copy link

miss-islington commented Jan 5, 2020

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

@miss-islington

This comment has been minimized.

Copy link

miss-islington commented Jan 5, 2020

I'm having trouble backporting to 3.8. Reason: 'Error 110 while writing to socket. Connection timed out.'. Please retry by removing and re-adding the needs backport to 3.8 label.

@miss-islington

This comment has been minimized.

Copy link

miss-islington commented Jan 5, 2020

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

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Jan 5, 2020

GH-17840 is a backport of this pull request to the 3.8 branch.

miss-islington added a commit that referenced this pull request Jan 5, 2020
tkinter.ttk.Scale().configure([name]) now returns a configuration tuple for name
or a list thereof for all options. Based on patch Giovanni Lombardo.
(cherry picked from commit 5ea7bb2)

Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
miss-islington added a commit that referenced this pull request Jan 5, 2020
tkinter.ttk.Scale().configure([name]) now returns a configuration tuple for name
or a list thereof for all options. Based on patch Giovanni Lombardo.
(cherry picked from commit 5ea7bb2)

Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
gousaiyang added a commit to gousaiyang/cpython that referenced this pull request Jan 8, 2020
tkinter.ttk.Scale().configure([name]) now returns a configuration tuple for name
or a list thereof for all options. Based on patch Giovanni Lombardo.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.