Fix pattern matching in side updates (running/progress/set_props) #2876
Conversation
T4rk1n
changed the title
| toPairs(outputs).forEach(([id, value]) => { | ||
| let componentId = id, | ||
| propName; | ||
| function parsePMCId(id: string) { |
There was a problem hiding this comment.
There's a lot of magic numbers in this function (+2, +1) -- could you add a comment explaining some of the string manipulations here?
There was a problem hiding this comment.
It's just the indexes of the strings, I think it's pretty readable if you know the format:
{"key":"value"}.propName or just the dictionary, so index refers to position of }, if the index + 2 < id.length means there is more data after the index and ., the propname starts at index + 2 and componentId is 0 to index + 1.
There was a problem hiding this comment.
got it, that makes sense, thanks! Could you add that format as a comment in the code?
There was a problem hiding this comment.
This looks duplicative of:
There was a problem hiding this comment.
This handle case where the prop name is not attached to the key, which comes with set_props so it searches for } instead of a dot.
There was a problem hiding this comment.
OK - can we at least put these in the same location - maybe move them both to a new file so it's easier to find them and we avoid making any more of them?
My main concern with doing this in slightly different ways is accidentally introducing additional constraints on what constitutes a valid ID, and/or causing an error with certain IDs. I think your logic here is OK, as you only get to searching for } after you already know the ID starts with { which is an invalid character for string IDs - but that's something we need to check carefully every time we modify the ID handling code.
There was a problem hiding this comment.
Thanks for putting the new function in a new file. The existing function is still in its old location though, so at some point please let's bring that one into the new file too.
| prevent_initial_call=True, | ||
| ) | ||
| def on_click(_) -> str: | ||
| time.sleep(1) |
There was a problem hiding this comment.
Can we do this with a lock instead of a delay?
|
I notice the running is not available/working with clientside callbacks, it could be working with a bit of refactor and I think could be useful when the clientside callback returns a promise. |
|
💯 |
Fix #2863
Fixes for cases where the given id has a
MATCH,ALL,ALLSMALLERwildcard in callbackrunningargument, progress for background callbacks andset_props.