Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-126008: Improve docstrings for Tkinter cget and configure methods#133303
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
…thods* Explain the behavior of Widget.configure() depending on arguments.* Unify descriptions.* Replace "resource" with "option".
| option(s) to have the given value(s); in this case the | ||
| command returns an empty string. The following options | ||
| are supported: | ||
| """Query or modify the configuration options for window TAGORID. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It took me a while to understand whatTAGORID meant, maybe it'd be better if we match the parameter name, i.e. usetarOrId (and similar for the other cases)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As I indicated elsewhere, I think the raw test should be*tagOrID* as italicizing option names is better than all-capping them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In THIS file, capitalization is used for parameter names. It is not ideal, but we should be consistent.
Lib/tkinter/__init__.py Outdated
| Actions are bound to events byresources (e.g. keyword argument | ||
| command) or with themethodbind. | ||
| Actions are bound to events byoptions (e.g. keyword argument | ||
| command) or with the bind() method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
'command' needs to be marked as literal string.
| command)orwiththebind()method. | |
| `command`)orwiththebind()method. |
Not sure if suggestion is correct, but as is, I initially read this 'keyword-option-command' as 1 compound noun.
Also, line 28, in the example, which is what got me to parse this correctly, needs 2 spaces after commas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What aboutthe command keyword argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
OK
| @@ -1827,18 +1827,24 @@ def _configure(self, cmd, cnf, kw): | |||
| # These used to be defined in Widget: | |||
| def configure(self, cnf=None, **kw): | |||
| """Configure resourcesofa widget. | |||
| """Query or modify the configuration optionsofthe widget. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| """Queryormodifytheconfigurationoptionsofthewidget. | |
| """Queryormodifytheconfigurationoptionsofwidget*self*. |
The expand text below is very nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
* and other markup is not used in docstrings. At least not in this file.
| return self.tk.call( | ||
| (self._w, 'itemcget') + (tagOrId, '-'+option)) | ||
| def itemconfigure(self, tagOrId, cnf=None, **kw): | ||
| """Configure resources of an item TAGORID. | ||
| """Query or modify the configuration options of an item TAGORID. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| """QueryormodifytheconfigurationoptionsofanitemTAGORID. | |
| """Queryormodifytheconfigurationoptionsofitem*tagOrID*. |
Just remembered that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
https://peps.python.org/pep-0257/#multi-line-docstrings says "Do not use the Emacs convention of mentioning the arguments of functions or methods in upper case in running text. Python is case sensitive and the argument names can be used for keyword arguments, so the docstring should document the correct argument names." 'tagOrID' is an example of why not. It goes on to suggest " It is best to list each argument on a separate line. For example:..." but this tends to bloat the doc. Don't add *s if you do not want, but please drop the all caps at least here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Using different convention for different methods will do more harm than good. This is a different issue, we will change the convention after fixing all docstrings.
| option(s) to have the given value(s); in this case the | ||
| command returns an empty string. The following options | ||
| are supported: | ||
| """Query or modify the configuration options for window TAGORID. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As I indicated elsewhere, I think the raw test should be*tagOrID* as italicizing option names is better than all-capping them.
When you're done making the requested changes, leave the comment: |
Uh oh!
There was an error while loading.Please reload this page.