Open to PRs for adding documentation and improved type hinting?

[washad]

I'd love to participate in this project.

One of the first things that I'd love to help with is to make the doc-strings more useful to others. I notice that a lot of methods are missing them entirely. Is that due to an inherent opposition to using them or is it simply an issue of resources. Said differently, would PRs improving docstrings be welcome?

Another improvement that I'd like to help with is to add some @properties to expose internals as read only, instead of needing to consume the entire dict. For example, I'd love to be able to just say "element.props", vs. "element.to_dict['props']".

Please let me know, I could start right away.

[falkoschindler]

Thanks for bringing this up and offering your help, @washad!

Documentation could clearly need some work. So PRs are very welcome. See also #242 for an (incomplete) list of already identified tasks.
When it comes to docstrings we primarily focussed on the public API, i.e. initializers and methods exposed via ui and app. But we should definitely add them for element methods like table.call_api_method() as well.
Private functions and API is documented sparingly to reduce clutter and the danger of documentation getting outdated. But in general we have no inherent opposition. 😉
If you start to create PRs, I'd suggest to keep them reasonable small. I don't mean to create one PR per docstring. But keeping different aspects separated (e.g. "docstrings for all public methods", "new example for xy", "new properties for xyz") helps reviewing and merging them quickly without too much conflicts with other branches.

Regarding element.props:

• Why would you need to access this dictionary?
• If you do need it, there is element._props. But sure, a that's not public.
• element.props is already the method to modify the props. So a public property would need a different name.

[washad]

Thank you so much for the quick response. I hope I didn't sound judgemental -- that was not my intent. This is a great product built on the back of free labor -- priorities must be established.

Based on your response, I'll try to get a PR out within the next day or so. As you correctly recommend, I'll keep it small. Not only will that allow easy review, but it will start the feedback loop and set expectations before I have written a lot of new code.

I'm thrilled at the opportunity to participate at making a great product greater. Thanks again for your time.

[washad]

Perhaps the first PR I'd like to submit, if ok with you, would be on the "Element" base class. Specifically, I'd like to add comments to the public methods and improve type hinting. For example, for the "on" method, I'd like to change the type hint of the "type" argument to ````````'Union[str,html_events]', where html_events is a Literal type that includes all possible events. Doing so will allow IDEs to auto-complete, saving the need to look for the docs?

[falkoschindler]

That's a great suggestion!
Regarding type hints for .on(): Maybe #117 is also of interest to you, especially my thoughts about auto-completion for literal strings. We might need to overload .on() to support type hints for both versions, literal strings and arbitrary strings.
And I'm not sure, could it be misleading to get the same list of suggested event names for all elements, even if some elements don't support all of them? And some elements also support Quasar events... 🫢

[washad]

"And I'm not sure, could it be misleading to get the same list of suggested event names for all elements"; Yeah, that might lessen the experience vs. improve it. I would imagine "on" methods in derived classes with more limited Literal type hints to address that. I can imagine a Literal hint inheritance architecture;

keyboard_events = Literal["onclick", "onmousedown", ....]
form_events = Literal["onblur", "onchange", ...]
drag_events = Literal["ondrag", "ondragend", ...]
html_events = Union[keyboard_events, form_events, drag_events]

[washad]

So here is kind of a taste of my style. Before I get too far, lemme know what you think;

from __future__ import annotations

import json
import re
from abc import ABC
from copy import deepcopy
from typing import TYPE_CHECKING, Any, Callable, Dict, List, Optional, Union

from . import background_tasks, binding, globals
from .elements.mixins.visibility import Visibility
from .event_listener import EventListener
from .events import handle_event
from .slot import Slot
from .typedefs import ElementAsDict

if TYPE_CHECKING:
    from .client import Client


PROPS_PATTERN = re.compile(r'([\w\-]+)(?:=(?:("[^"\\]*(?:\\.[^"\\]*)*")|([\w\-.%:\/]+)))?(?:$|\s)')


# noinspection PySingleQuotedDocstring
class Element(ABC, Visibility):
    '''
    Define the base class for all html elements represented as Python objects.

    :param tag: The html tag (e.g. div, p, etc. of the element.)
    '''

    def __init__(self, tag: str, *, _client: Optional[Client] = None) -> None:
        super().__init__()
        self.client = _client or globals.get_client()
        self.id = self.client.next_element_id
        self.client.next_element_id += 1
        self.tag = tag
        self._classes: List[str] = []
        self._style: Dict[str, str] = {}
        self._props: Dict[str, Any] = {}
        self._event_listeners: List[EventListener] = []
        self._text: str = ''
        self.slots: Dict[str, Slot] = {}
        self.default_slot = self.add_slot('default')

        self.client.elements[self.id] = self
        self.parent_slot: Optional[Slot] = None
        slot_stack = globals.get_slot_stack()
        if slot_stack:
            self.parent_slot = slot_stack[-1]
            self.parent_slot.children.append(self)

    def add_slot(self, name: str) -> Slot:
        self.slots[name] = Slot(self, name)
        return self.slots[name]

    def __enter__(self) -> 'Element':
        '''Allow element to be used as a context manager (with statement.)'''
        self.default_slot.__enter__()
        return self

    def __exit__(self, *_):
        self.default_slot.__exit__(*_)

    def to_dict(self) -> ElementAsDict:
        '''Get important attributes of an element as a dictionary.'''
        events: Dict[str, Dict] = {}
        for listener in self._event_listeners:
            words = listener.type.split('.')
            type = words.pop(0)
            specials = [w for w in words if w in {'capture', 'once', 'passive'}]
            modifiers = [w for w in words if w in {'stop', 'prevent', 'self', 'ctrl', 'shift', 'alt', 'meta'}]
            keys = [w for w in words if w not in specials + modifiers]
            events[listener.type] = {
                'listener_type': listener.type,
                'type': type,
                'specials': specials,
                'modifiers': modifiers,
                'keys': keys,
                'args': list(set(events.get(listener.type, {}).get('args', []) + listener.args)),
                'throttle': min(events.get(listener.type, {}).get('throttle', float('inf')), listener.throttle),
            }
        return {
            'id': self.id,
            'tag': self.tag,
            'class': self._classes,
            'style': self._style,
            'props': self._props,
            'events': events,
            'text': self._text,
            'slots': {name: [child.id for child in slot.children] for name, slot in self.slots.items()},
        }

    def classes(self, add: Optional[str] = None, *, remove: Optional[str] = None, replace: Optional[str] = None) \
            -> 'Element':
        '''
        Apply, remove, or replace HTML classes.

        Generally, this is for the purpose of modifying the look of the element or its layout, based on the
        Quasar framework.

        .. note:: Removing classes can be helpful if default NiceGUI class-styling is not desired.

        :param add: a white-space delimited string of classes
        :param remove: A white-space delimited string of classes to remove from the element.
        :param replace: A white-space delimited string of classes to use instead of existing.
        '''
        class_list = self._classes if replace is None else []
        class_list = [c for c in class_list if c not in (remove or '').split()]
        class_list += (add or '').split()
        class_list += (replace or '').split()
        new_classes = list(dict.fromkeys(class_list))  # NOTE: remove duplicates while preserving order
        if self._classes != new_classes:
            self._classes = new_classes
            self.update()
        return self

    @staticmethod
    def _parse_style(text: Optional[str]) -> Dict[str, str]:
        result = {}
        for word in (text or '').split(';'):
            word = word.strip()
            if word:
                key, value = word.split(':', 1)
                result[key.strip()] = value.strip()
        return result

    def style(self, add: Optional[str] = None, *, remove: Optional[str] = None, replace: Optional[str] = None)\
            -> 'Element':
        '''
        Apply, remove, or replace CSS style sheet definitions to modify the look of the element.

        .. note::
            Removing styles can be helpful if the predefined style sheet definitions by NiceGUI are not wanted
            in a particular styling.

        .. codeblock:: python

            my_btn=Button("MyButton).style("color: #6E93D6; font-size: 200%", remove="font-weight; background-color")

        :param add: A semicolon separated list of styles to add to the element.
        :param remove: A semicolon separated list of styles to remove from the element.
        :param replace: Like add, but existing styles will be replaced by given.
         '''
        style_dict = deepcopy(self._style) if replace is None else {}
        for key in self._parse_style(remove):
            if key in style_dict:
                del style_dict[key]
        style_dict.update(self._parse_style(add))
        style_dict.update(self._parse_style(replace))
        if self._style != style_dict:
            self._style = style_dict
            self.update()
        return self

    @staticmethod
    def _parse_props(text: Optional[str]) -> Dict[str, Any]:
        dictionary = {}
        for match in PROPS_PATTERN.finditer(text or ''):
            key = match.group(1)
            value = match.group(2) or match.group(3)
            if value and value.startswith('"') and value.endswith('"'):
                value = json.loads(value)
            dictionary[key] = value or True
        return dictionary

    def props(self, add: Optional[str] = None, *, remove: Optional[str] = None) -> 'Element':
        '''Add or remove Quasar-specif properties to modify the look of the element.

        see https://quasar.dev/vue-components/button#design

        .. code:: python

            by_btn = Button("my_btn").props("outline icon=volume_up")

        .. note:: Boolean properties are assumed True by their existence.

        :param add: A whitespace separated list of either boolean values or key=value pair to add
        :param remove: A whitespace separated list of property keys to remove.
        '''
        needs_update = False
        for key in self._parse_props(remove):
            if key in self._props:
                needs_update = True
                del self._props[key]
        for key, value in self._parse_props(add).items():
            if self._props.get(key) != value:
                needs_update = True
                self._props[key] = value
        if needs_update:
            self.update()
        return self

    def tooltip(self, text: str):
        with self:
            tooltip = Element('q-tooltip')
            tooltip._text = text
        return self

    def on(self, type: str, handler: Optional[Callable], args: Optional[List[str]] = None, *, throttle: float = 0.0) \
            -> 'Element':
        if handler:
            args = args if args is not None else ['*']
            listener = EventListener(element_id=self.id, type=type, args=args, handler=handler, throttle=throttle)
            self._event_listeners.append(listener)
        return self

    def handle_event(self, msg: Dict) -> None:
        for listener in self._event_listeners:
            if listener.type == msg['type']:
                handle_event(listener.handler, msg, sender=self)

    def collect_descendant_ids(self) -> List[int]:
        '''
        Return a list of ids of the element and each of its descendents.

        ... note:: The first id in the list is that of the element.
        '''
        ids: List[int] = [self.id]
        for slot in self.slots.values():
            for child in slot.children:
                ids.extend(child.collect_descendant_ids())
        return ids

    def update(self) -> None:
        if not globals.loop:
            return
        ids = self.collect_descendant_ids()
        elements = {id: self.client.elements[id].to_dict() for id in ids}
        background_tasks.create(globals.sio.emit('update', {'elements': elements}, room=self.client.id))

    def run_method(self, name: str, *args: Any) -> None:
        if not globals.loop:
            return
        data = {'id': self.id, 'name': name, 'args': args}
        background_tasks.create(globals.sio.emit('run_method', data, room=globals._socket_id or self.client.id))

    def clear(self) -> None:
        '''Remove all descendant (child) elements.'''
        descendants = [self.client.elements[id] for id in self.collect_descendant_ids()[1:]]
        binding.remove(descendants, Element)
        for element in descendants:
            del self.client.elements[element.id]
        for slot in self.slots.values():
            slot.children.clear()
        self.update()

    def remove(self, element: Union[Element, int]) -> None:
        '''
        Remove a descendant (child) element.

        :param element: Either the element instance or its id.
        '''
        if isinstance(element, int):
            children = [child for slot in self.slots.values() for child in slot.children]
            element = children[element]
        binding.remove([element], Element)
        del self.client.elements[element.id]
        for slot in self.slots.values():
            slot.children[:] = [e for e in slot.children if e.id != element.id]
        self.update()

    def delete(self) -> None:
        """Called when the corresponding client is deleted.

        Can be overridden to perform cleanup.
        """

file: typedefs.py (here is where the event Literals would go.)

"""
Define types used throughout to assist with auto-complete.
"""
from typing import List, Dict, Any

from typing_extensions import TypedDict

# note: This element of typed dict is necessary due to the use of the "class" keyword.
# note: Ideally the dict types get better refined moving forward to give more clarity to the caller.
ElementAsDict = TypedDict('ElementAsDict', {
    'id': int,
    'tag': str,
    'class': List[str],
    'style': Dict[str, str],
    'props': Dict[str, Any],
    'events': Dict[str, Dict],
    'text': str,
    'slots': Dict
})

[falkoschindler]

Oh and I didn't annotate the return type of __enter__, classes, style, ... because I didn't know how. Isn't Element wrong, since a derived class will return its type and not Element? We'd need PEP 673, which is only available in Python 3.11. Maybe typing-extensions helps?

[washad]

"We'd need PEP 673, which is only available in Python 3.11. Maybe typing-extensions helps?", it does indeed appear to be supported in typing-extensions 🎉 , That is pretty awesome : )

[washad]

"There are some methods like to_dict","So I'm not sure if they should get an underscore," I'm of the personal opinion that, for libraries, the consumer (user) should be protected from knowing things they shouldn't need to know and having any more choices than is absolutely needed, as choices are confusing. Having to decide which auto-complete popup is relevant and which is not usually makes the tool more difficult to use. I'm a fan of encapsulating the knowledge (for what little my opinion is worth) via underscore.

[washad]

"The docstring lines with .. ". These are RST directives, specified in the specs. I'm surprised that VSCode struggles with them. Sphinx handles them well. I can use another method if you like; https://docutils.sourceforge.io/docs/ref/rst/directives.html

[falkoschindler]

Let's continue our discussion over at #356.

[washad]

"The first line of a docstring should follow the triple quotes immediately without a linebreak" I didn't notice a makefile or justfile with a linting target. Does automated linting exist and I am missing it? The reason I ask is that some of the popular linters force mult-line docstrings into a new line, so they complain at me if I write it as you suggest. Maybe if you have some preferred linters I'll save you some time by running those before the PR? If not, and there is interest, maybe automated linting is something I can offer later, as well.

[falkoschindler]

Let's continue our discussion over at #356.