modify st.echo to use ast to handle indenting properly #5375
Conversation
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Do you think we should add tests for this change? |
|
The fix replaces the logic for how to determine the code to be echoed. The existing st.echo tests should cover the fix, no? |
|
@sfc-gh-brianhess I don't think there are any test cases that cover the case you're fixing, so it seems like it would be good to add one to https://github.com/streamlit/streamlit/blob/develop/lib/tests/streamlit/elements/echo_test.py |
|
Added test case |
|
Hello, Brain [ @sfc-gh-brianhess ], thank you very much for your contribution, it really looks great! Unfortunately We should either wait until we stop supporting Python 3.7 for Streamlit, or have a decision to maintain different behavior based on the Python version for |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Since we're now no longer officially supporting 3.7, this should be good to go, right @kajarenc? |
|
@blackary yes, I looked at this PR just after removing 3.7 from the list now we have some conflicts in the tests with the current But I definitely take a look at it again in the near future, and will try to resolve conflicts and merge it! |
lib/streamlit/echo.py
Outdated
|
|
||
| ap = ast.parse("".join(source_lines)) | ||
|
|
||
| ap_map = {} |
There was a problem hiding this comment.
The purpose/use of this map is not very clear from the name
| def do_y(self): | ||
| pass""" | ||
|
|
||
| element = self.get_delta_from_queue(echo_index).new_element |
There was a problem hiding this comment.
Tests using get_delta_from_queue could also be written with the interactive script tests framework, which might be clearer, though this is not required.
AnOctopus
left a comment
There was a problem hiding this comment.
The AST traversal code isn't the most obvious (though it wasn't that hard to follow after I pulled up the ast module docs), so could benefit from better naming and/or explanatory comments.
* develop: Respect current existing env var in build_info.py (streamlit#6906) fix: frontend/package.json & frontend/yarn.lock to reduce vulnerabilities (streamlit#6959) Update hamburger menu icon to overflow (streamlit#6947) Fix: description of config options (streamlit#6917) Add missing border radius (streamlit#6944) Up version to 1.24.0 (streamlit#6905) modify st.echo to use ast to handle indenting properly (streamlit#5375)
st.echo() has a simplistic approach to figuring out what to consider inside the echo(). It actually has issues with some use cases entirely, such as:
with st.echo(
location="below"
):
st.markdown("# Hiya")
Instead, we should use ast to properly parse the file and output the proper full with st_echo() block.
📚 Context
Please describe the project or issue background here
st.echo() has a simplistic approach to figuring out what to consider inside the echo(). It actually has issues with some use cases entirely, such as:
Instead, we should use
astto properly parse the file and output the proper fullwith st_echo()block.What kind of change does this PR introduce?
🧠 Description of Changes
Add bullet points summarizing your changes here
use ast to parse the file and find the block for the st.echo() properly
Revised:
Insert screenshot of your updated UI/code here
Current:
Insert screenshot of existing UI/code here
🧪 Testing Done
🌐 References
Does this depend on other work, documents, or tickets?
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.