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

Add filtering to RecallContextManager #1095

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Add filtering to RecallContextManager #1095

wants to merge 3 commits into from

Conversation

wch
Copy link
Collaborator

@wch wch commented Feb 1, 2024

This PR implements the change mentioned in #1083 (comment) and the following comment.

Some notes and things that remain to be done:

  • The UiRecallContextManager class might not actually necessary, because it simply adds a different default filter function.
  • The filter_ui_objects function is meant to return True for TagChild or TagAttrs or None objects. However, there's not really a good way to check if something is a TagChild or TagAttrs, since those aren't classes, but instead Union types which are recursive. I suppose we could recurse into the objects and check the entire tree to see if everything fits into the type, but that seems pretty crude. This issue is the most questionable part of this approach.
  • Some of the UI functions accept just TagChild args, while others accept TagChild | TagAttrs, but the filter_ui_objects function currently only checks for the latter. There should be a different filter function that checks for the former.
  • When errors happen, print better error messages, with line number and text.

@wch wch requested a review from jcheng5 February 1, 2024 16:47
@jcheng5
Copy link
Collaborator

jcheng5 commented Feb 1, 2024

@wch and I met on this but it was cut short. My notes:

  1. For this release, the behavior we agreed to was that an error would still be thrown, but it would identify the file and line number (and ideally, code) that caused the problem. (It would be good if the code was printed at the console but it is important that the code is NOT rendered in the browser, for security/privacy)
  2. In order to accomplish ☝️ we need to throw at the time the problematic object is encountered, not when the enclosing context manager exits. So filter or something like it is still needed.
  3. Rather than filter--which can only allow, ignore, or throw--I'd prefer a method override. This would still permit allow, ignore, and throw, but also "proceed but with modified input", "proceed but I'll modify the return value", "execute more than once", etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants