Widgets with identical IDs raise an Exception#425
Widgets with identical IDs raise an Exception#425tconkling merged 7 commits intostreamlit:developfrom
Conversation
monchier
left a comment
monchier
left a comment
There was a problem hiding this comment.
It would be nice to have this be some "base" property of a Widget, so that we don't have to go and test each individual widget.
Not sure this is feasible or what is a good strategy. Maybe could go in one of these decorators we have in DeltaGenerator.py?
Looks good to me otherwise.
| added = ctx.widget_ids_this_run.add(widget_id) | ||
| if not added: |
There was a problem hiding this comment.
It is not straightforward that added is None means that the widget id is duplicated. Maybe raise the exception inside add(?
There was a problem hiding this comment.
I (softly!) disagree - I have add() returning a bool (not a None) indicating whether the item was successfully added, which mirrors the API in other languages (e.g. https://docs.microsoft.com/en-us/dotnet/api/system.collections.generic.hashset-1.add?view=netframework-4.8).
I'm hesitant to add an Exception to indicate this case; it feels like it violates the "Never use exception for flow-control" principle. The item existing in the set already is not (IMO) an exceptional case - the exceptional case is when the user has defined a duplicate key, but this is the concern of the DeltaGenerator function, not the set it uses to detect this.
That said, I don't disagree so strongly that I wouldn't back down! Does anyone else in @streamlit/eng have an opinion here?
There was a problem hiding this comment.
I think I come to agree with you. I missed it is returning a bool.
|
|
||
|
|
||
| class _WidgetIDSet(object): | ||
| """Stores a set of widget IDs. Safe to mutate from multiple threads.""" |
There was a problem hiding this comment.
I have been told to use
"""Stores a set of widget IDs. Safe to mutate from multiple threads.
"""
See the newline.
There was a problem hiding this comment.
My understanding is that this rule only applies when the docstring is multiline. For singleline docstrings, the closing triple-quote goes on the same line, per https://www.python.org/dev/peps/pep-0257/#one-line-docstrings
| "button": lambda key=None: st.button("", key=key), | ||
| "checkbox": lambda key=None: st.checkbox("", key=key), | ||
| "multiselect": lambda key=None: st.multiselect("", options=[1, 2], key=key), | ||
| "radio": lambda key=None: st.radio("", options=[1, 2], key=key), | ||
| "selectbox": lambda key=None: st.selectbox("", options=[1, 2], key=key), | ||
| "slider": lambda key=None: st.slider("", key=key), | ||
| "text_area": lambda key=None: st.text_area("", key=key), | ||
| "text_input": lambda key=None: st.text_input("", key=key), | ||
| "time_input": lambda key=None: st.time_input("", key=key), | ||
| "date_input": lambda key=None: st.date_input("", key=key), |
There was a problem hiding this comment.
How are we going to remember to add new widgets here?
There was a problem hiding this comment.
I'm not sure there's a good mechanism for this, other than vigilance? That is, anyone adding a new widget should be following the conventions established by all the other widgets.
We don't currently annotate any of the widget creation functions (and I believe @tvst is hoping to move away from our more deeply-nested decorator abuse within DeltaGenerator?). But: open to suggestions!
There was a problem hiding this comment.
Yeah... I see abuse in the decorator pattern. I feel having all this logic in DeltaGenerator is a bit overwhelming... Maybe better class structure? I am hesitant to propose a "base" class/"derived" class mechanism or an equivalent composition pattern, but it may work here.
There was a problem hiding this comment.
I'll open a ticket to start thinking about how this might be improved!
2 participants
ReportContexthas a newwidget_ids_this_runset. Each time a widget has an ID assigned, we add that ID to the set. If the ID was already in the set, the exception is thrown. (Because it's possible to access this set from multiple threads, access to it is wrapped in athreading.Lock).ScriptRunnerclears thewidget_ids_this_runright before running its script."{widget_type}-{user-key}"). So this allows two widgets with different types to share the same user key. Easy to change if this is undesirable!Fixes #40