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
Tkinter.Scrollbar: the activate method needs to return a value, and set should take only two args #50417
Comments
Hi, I've noticed some minor problems in Tkinter.Scrollbar that would be good The second problem is about the set method. It accepts any amount of |
A tiny patch, can someone take a look with a view to committing, thanks. |
(1) Why did you change the name of the parameter from index to element? If the underlying engine also accepts indices (e.g., self.activate(4) ) then the name should stay. If it really is only meaningful to use the name of an element -- maybe it should still stay for backwards compatibility. Or at least accept the old name too for a release. Either way, please provide a test case showing that it works under the new name; there may also be doc fixes. (I'm not sure there is documentation for this widget, though, and providing some in the first place is good, but perhaps a different task.) FWIW, (2) It looks like the set command took *args to give some freedom. There may be extensions that take a third argument. It may well be valid to call it without any arguments, or with only one. So the signature may turn into set(first=0, last=1, *args) or some such. Whatever the answer about what arguments are really needed, there should be test cases to demonstrate this before the API is changed. |
In versions of Tk before 4.0, the set command accepted 4 arguments. I think this is a new feature and can be applied only to 3.4. Agree with Jim that for backward compatibility we should keep name "index" and arbitrary number of arguments at least for one release (with deprecation warnings etc). |
I withdraw my comment about arbitrary number of arguments. Tk itself raises an exception if the number of arguments is wrong. |
The patch updated to tip and added deprecation warning for the "index" keyword argument. |
I'm still not seeing why these changes are sufficiently desirable to justify the code churn. Nor am I seeing test or doc changes that would explain the advantages of the new way, and prevent future regressions. I agree that the changes would make the signatures better for the typical use cases for this particular widget -- but wouldn't they also break the common interface for the "set" and "activate" methods across several types of tkinter widget? If so, then instead of changing or restricting the method, it would be better to add examples (and maybe even an explanation) to the documentation (including the docstring). In particular: (1) Why change actrivate's parameter from "index" to "element"? I agree that "element" is a better name for the normal case, but (2) Why change the "set" method? I understand that a more specific signature is desirable, and I assume that other values would be ignored (or raise an exception), but the set method seems to have an existing API across several widgets -- and that shouldn't be broken lightly. |
Does msg201484 mean that tk requires exactly 2 args? If so, some change seems ok. Deleting 'args' cannot in itself break code as 'args' cannot be used as a keyword. I agree with not adding defaults, |
There is no the common interface for the "set" and "activate" methods. Listbox.activate(index) - mandatory argument is an index (an integer, "active", "anchor", "end", or @x,y form). |
Here is a patch with added tests. |
Indices have special meaning in Tk. INDICES
An argument of Scrollbar.activate() obviously is not an index. On other hand, the "element" parameter is used consistently in other methods: Spinbox.invoke() and Spinbox.selection_element(). About the set method Tk inter documentation says:
It would be better to use same parameter names as in Tk. I am planning to update all Tkinter docstrings from more detailed Tk documentation in separate issue. |
A Spinbox is not a Listbox. The common feature of the activate methods you listed is that the parameter is called 'index'. But I think this is a moot point. To the best of my knowledge, casually changing parameter names for no functional benefit is against policy. The case for doing so is much weaker than for the re method parameter mess (correct module?). The current discussion about turtle.py on pydev reinforces my impression. Please drop the idea or ask for policy clarification on pydev. |
New changeset 6ae34a948cb4 by Serhiy Storchaka in branch 'default': New changeset f5df571bfe1d by Serhiy Storchaka in branch '2.7': New changeset 2cac1e3f825a by Serhiy Storchaka in branch '3.4': |
Committed without change activate() parameter name. I still think this parameter name is wrong. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: