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

Returning self instead of None for Widget "verb" methods #1908

Closed
davep opened this issue Mar 1, 2023 Discussed in #1817 · 2 comments
Closed

Returning self instead of None for Widget "verb" methods #1908

davep opened this issue Mar 1, 2023 Discussed in #1817 · 2 comments
Assignees
Labels
enhancement New feature or request Task

Comments

@davep
Copy link
Contributor

davep commented Mar 1, 2023

Discussed in #1817

Originally posted by liancheng February 16, 2023
My app has a form-like interface, and bound one key to each widget to set focus and optionally adjust the input, e.g., toggle a switch, move the cursor to the next ListItem in a ListView, etc.

For Switches, I wrote the following method to achieve this:

def action_toggle_switch(self, selector: str) -> None:
    switch = self.query_one(selector, Switch)
    switch.focus()
    switch.toggle()

The fact that both focus() and toggle() return None prevented me from chaining the calls. If they returned self instead, the above method can be simplified to:

def action_toggle_switch(self, selector: str) -> None:
    self.query_one(selector, Switch).focus().toggle()

I'm not quite sure whether this violates certain well-established Python best practices or conventions, though.

Chatted with @willmcgugan about this and the general feeling is this would be a good thing to do (if done carefully).

@davep davep added enhancement New feature or request Task labels Mar 1, 2023
@davep davep changed the title Returning self instead of None for Widget "verb" methods Returning self instead of None for Widget "verb" methods Mar 1, 2023
@rodrigogiraoserrao rodrigogiraoserrao self-assigned this Mar 22, 2023
rodrigogiraoserrao added a commit that referenced this issue Mar 22, 2023
I compiled a list of all widget methods that return 'None' and for which it _could_ make sense to make this change.
(I filtered out some methods, like watch and action methods.)

I tried choosing a subset of those methods, trying to only pick methods for which there weren't two things that could be returned (e.g., 'Widget.move_child' _could_ return either the widget or the child that was moved) and I also tried to only pick methods that have little or no parameters (e.g., 'Widget.animate' has many parameters and is typically called with quite a few.

These are all the 'Widget' methods for which this could make sense:
- 'move_child' (either return 'self' or the actual 'child' that was moved…)
- 'animate'
- 'scroll_to' / 'scroll_relate' / 'scroll_home' / 'scroll_end' / 'scroll_left' / 'scroll_right' / 'scroll_down' / 'scroll_up' / 'scroll_page_up' / 'scroll_page_down' / 'scroll_page_left' / 'scroll_page_right' / 'scroll_visible'
- 'refresh'
- 'focus' / 'reset_focus'
- 'capture_mouse' / 'release_mouse'

Additionally, I looked at each widget, and found these methods:
- 'Tree'
    - 'TreeNode'
        - 'expand' / 'expand_all' / 'collapse' / 'collapse_all' / 'toggle' / 'toggle_all'
        - 'set_label'
    - 'clear' / 'reset'
    - 'select_node' (either return 'self' or the actual 'node' that was selected)
    - 'scroll_to_line' / 'scroll_to_node'
    - 'refresh_line'
- 'ToggleButton'
    - 'toggle' (and 'action_toggle'?)
- 'TextLog'
    - 'write'
    - 'clear'
- 'Tabs'
    - 'add_tab' / 'remove_tab'
    - 'clear'
- 'Switch'
    - 'toggle' (and 'action_toggle'?)
- 'Static'
    - 'update'
- 'Pretty'
    - 'update'
- 'Placeholder'
    - 'cycle_variant'
- '_markdown.py'
    - 'MarkdownBlock'
        - 'set_content'
    - 'MarkdownTableOfContents'
        - 'set_table_of_contents'
- 'Input'
    - 'insert_text_at_cursor'
- 'DirectoryTree'
    - 'load_directory'
- 'DataTable'
    - 'update_cell' / 'update_cell_at'
    - 'clear'
    - 'refresh_coordinate' / 'refresh_row' / 'refresh_column'
    - 'sort'
- 'Button'
    - 'press'

Related issues: #1908
Related discussions: #1817
@rodrigogiraoserrao
Copy link
Contributor

This was implemented in #2102.

@github-actions
Copy link

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Task
Projects
None yet
Development

No branches or pull requests

2 participants