Make st.help more resilient with conditional members #8228
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
lib/streamlit/elements/doc_string.py
Outdated
| else: | ||
| member_docs = None | ||
| member_value = human_readable_value | ||
| except Exception: |
There was a problem hiding this comment.
I set any exception vs just AttributeError to be extra cautious
There was a problem hiding this comment.
I'm worried that doing something like this might swallow legitimate errors rather than just the ones that are normal/expected
Would probably prefer to only exclude the expected AttributeError case here
|
|
||
| def __getattribute__(self, name): | ||
| if name == "say_hello" and not self.available: | ||
| raise AttributeError(f"{name} is not accessible when x is even") |
There was a problem hiding this comment.
nit: the error message here seems unrelated to how we actually decide whether to throw an error in this method
| el = self.get_delta_from_queue().new_element.doc_string | ||
| self.assertEqual("ConditionalHello", el.type) | ||
| member_names = [member.name for member in el.members] | ||
| self.assertTrue("say_hello" in member_names) |
There was a problem hiding this comment.
nit (here and below): I think there's an assertIn method that makes this more explicit
| return object.__getattribute__(self, name) | ||
|
|
||
| def say_hello(self): | ||
| print("Hello") |
There was a problem hiding this comment.
super nit: doesn't really matter since this method is never called, but it might be slightly preferable to have the body of this method be a no-op to not have a print statement in the code
## Describe your changes `st.help` leverages `dir(object)` to determine the members in the object to expose to the docstring. If an object makes a member conditional based on internal state, `st.help` can throw an `AttributeError`. Ideally, in these cases, the developer of the object would implement the `__dir__` method to handle conditional members, but Streamlit can be more resilient in handling this situation. The example that brought this up was in sklearn, and I created scikit-learn/scikit-learn#28558 with them to better handle this scenario. ## Testing Plan - Unit Tests should cover the change with the member available and unavailable --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
Describe your changes
st.helpleveragesdir(object)to determine the members in the object to expose to the docstring. If an object makes a member conditional based on internal state,st.helpcan throw anAttributeError. Ideally, in these cases, the developer of the object would implement the__dir__method to handle conditional members, but Streamlit can be more resilient in handling this situation.The example that brought this up was in sklearn, and I created scikit-learn/scikit-learn#28558 with them to better handle this scenario.
Testing Plan
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.