Skip to content

Conversation

evnchn
Copy link
Collaborator

@evnchn evnchn commented Jun 1, 2025

Motivation

Addresses NiceGUI Wishlist

Implementation

  • I tried click_events: List[str] = ('click', 'dblclick'), but that would cause mypy to complain.
  • click_events: Sequence[str] = ('click', 'dblclick') could be doable, but we are changing List to Sequence, not sure if that'd be a breaking change
  • Wound up asking Copilot to "no mutable default" and its solution was click_events: Optional[List[str]] = None with if click_events is None: click_events = ['click', 'dblclick']

Progress

  • I chose a meaningful title that completes the sentence: "If applied, this PR will..."
  • The implementation is complete.
  • Pytests have been added (or are not necessary).
  • Documentation has been added (or is not necessary).

Tests are green at https://github.com/evnchn/nicegui/actions/runs/15380276062, but the point about whether this is a breaking change is still worthy.

@evnchn evnchn requested a review from Copilot June 1, 2025 23:39
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR removes mutable default arguments (B006 warnings) by changing list/dict defaults to None and initializing them inside constructors or subclass hooks.

  • Replace mutable defaults (lists/dicts) with Optional[...] = None in multiple element constructors
  • Add if ... is None: checks to lazily initialize defaults
  • Update __init_subclass__ in element.py to apply the same pattern for class-level lists

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
nicegui/elements/scene.py Made click_events optional and added lazy initialization
nicegui/elements/markdown.py Made extras optional and added lazy initialization
nicegui/elements/leaflet.py Made options optional and added lazy initialization
nicegui/elements/keyboard.py Made ignore optional and added lazy initialization
nicegui/elements/interactive_image.py Made events optional and added lazy initialization
nicegui/elements/aggrid.py Made html_columns and options optional and added init logic
nicegui/element.py Changed class-level list defaults to optional and fallback init
Comments suppressed due to low confidence (4)

nicegui/element.py:99

  • [nitpick] Using dependencies = dependencies or [] may unintentionally replace an explicit empty list; consider if dependencies is None: dependencies = [] for clearer intent and to avoid edge cases with falsy values.
dependencies = dependencies or []

nicegui/elements/aggrid.py:59

  • The Dict type is unparameterized here, which limits type safety; consider using Dict[str, Any] (or a more specific signature) to improve clarity and static analysis.
options: Optional[Dict] = None

nicegui/elements/leaflet.py:30

  • Similar to other parts of the codebase, this Dict could be parameterized (e.g. Dict[str, Any]) to provide better type hints and prevent misuse at call sites.
options: Optional[Dict] = None,

nicegui/element.py:100

  • [nitpick] For consistency and clarity, use an explicit if libraries is None: check instead of libraries or [] to initialize the default list.
libraries = libraries or []

@@ -13,7 +13,7 @@ class Markdown(ContentElement, component='markdown.js', default_classes='nicegui

def __init__(self,
content: str = '', *,
extras: List[str] = ['fenced-code-blocks', 'tables'], # noqa: B006
extras: Optional[List[str]] = None,
Copy link
Preview

Copilot AI Jun 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring still describes the default extras as ['fenced-code-blocks', 'tables']; consider updating it to note that passing None results in those defaults being applied internally.

Copilot uses AI. Check for mistakes.

@falkoschindler
Copy link
Contributor

click_events: Sequence[str] = ('click', 'dblclick') could be doable, but we are changing List to Sequence, not sure if that'd be a breaking change

In my point of view this is not a breaking change because we're loosening the type constraint. Since List is a Sequence, existing user code is still valid. Note that return types would work the other way around: Changing Sequence to the more specific List would be ok, but going from List to Sequence would be a breaking change because the user might rely on the return value being mutable which Sequence isn't guaranteed to be.

In my opinion, using Optional is a tiny bit confusing because the user might think: "Ok, it can be a List or None. I don't want any click events. So lets pass None." But None leads to two default click arguments, not an empty list.

And Sequence has the advantage that the user can pass tuples. This is considered good practice for type annotation: accept broad, return narrow.

@evnchn
Copy link
Collaborator Author

evnchn commented Jun 2, 2025

Interesting.

How about Dict? No frozendict. MappingProxyType? https://adamj.eu/tech/2022/01/05/how-to-make-immutable-dict-in-python/

@evnchn
Copy link
Collaborator Author

evnchn commented Jun 2, 2025

Also, if I understand correctly, since tuples can alo be Sequence, then it means that we no longer can leverage append or mutate the input in any way, Thus, we'd need to copy it to a new list if we need to mutate it, or we need to change our code to never mutate, right?

@falkoschindler
Copy link
Contributor

falkoschindler commented Jun 2, 2025

How about Dict?

Good question. options: Mapping: MappingProxyType({}) seems to be technically correct. I just hope that it doesn't confuse anyone too much.

Actually, one reason why we kept using List and Dict instead of Sequence and Mapping was that it feels more readable for beginners who might not know these advanced datatypes.

Another aspect I haven't though of: If someone derived, e.g., ui.interactive_image and assumes the events parameter is of type List, changing it to Sequence would indeed break the derived class. Therefore we need to wait for 3.0.

Thus, we'd need to copy it to a new list if we need to mutate it, or we need to change our code to never mutate, right?

That's the point of the whole exercise. Our code should never mutate such input parameters because their default value is "static". If one ui.interactive_image would append something to the default events list, it would affect the default value of all other ui.interactive_images. (See https://docs.astral.sh/ruff/rules/mutable-argument-default/.)

As far as I can tell, within the NiceGUI library we already never mutate default arguments. But user code could - accidentally or intentionally - break this convention, allowing for nasty bugs down the line. By changing the signatures to immutable types, we enforce derived classes to follow this rule as well.

@evnchn evnchn added the ⚪️ minor Priority: Low impact, nice-to-have label Jun 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚪️ minor Priority: Low impact, nice-to-have
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants