Improve st.help (and st.write's usage of st.help!) #5857
Conversation
| # with contextlib.suppress(ValueError): | ||
| # sig = str(inspect.signature(obj)) |
Check notice
Code scanning / CodeQL
Commented-out code
|
Had a rough look through it and generally LGTM! Let's make a demo app in the feature-demos repo before we merge, so we can have a look what it looks like in live, and then I'll give my final LGTM. |
(i.e. it works but doesn't show variable name)
(this typically only happens in PR previews)
|
@sfc-gh-tteixeira What happens if I do |
Turns out the feature-demos app doesn't work very well with this. That's beacuse part of the new So I'm adding a warning to e2e/scripts/st_help.py to explain this, so people don't get confused when they run feature-demos.
|
|
Hey, @vdonato, maybe you can help me with this... I can't run the e2e tests locally on my Mac, so update the screenshots. I'm getting this error:
I'm running this in a Docker container with The logs above were captured by running I tried updating the deps as mentioned at https://on.cypress.io/required-dependencies, but that doesn't do it either. Any idea what's up? Could this be an ARM thing? |
|
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. |
f7a2f40 to
a5fe440
Compare
|
@sfc-gh-tteixeira whoops, sorry! The @mention from a few weeks back must have gotten lost in a sea of emails, so I didn't notice it until a later email notification for this PR. I haven't updated snapshots locally via the Docker image for quite awhile at this point (I've mostly been deleting the snapshots to allow cypress running in CI to regenerate them for me), although I was under the impression that doing so still worked given that a few changes to the Dockerfile have been made relatively recently (CC @sfc-gh-kbregula and @sfc-gh-tszerszen as I believe they recently made some related changes). I'm not sure if folks still using the local docker test env are using M1 machines, though. In either case, I think the path being taken in #6006 would likely be the preferred improvement here, anyway, given that attempting to replicate the exact environment used in CI to take snapshots locally has always been extremely fragile / oftentimes slower than using CI when updating tons of snapshots due to the lack of parallelization. |
|
I'm using Docker image on Mac OS x86, but building the image currently doesn't work because there is a problem with deleted options in To build the dockerfile, you need to add one patch locally. diff --git a/Makefile b/Makefile
index 5b39d4606..a10b9bcb4 100644
--- a/Makefile
+++ b/Makefile
@@ -83,9 +83,9 @@ SHOULD_INSTALL_TENSORFLOW := $(shell python scripts/should_install_tensorflow.py
py-test-install: lib/test-requirements.txt
# As of Python 3.9, we're using pip's legacy-resolver when installing
# test-requirements.txt, because otherwise pip takes literal hours to finish.
- pip install -r lib/test-requirements.txt --use-deprecated=legacy-resolver
+ pip install -r lib/test-requirements.txt
ifeq (${SHOULD_INSTALL_TENSORFLOW},true)
- pip install -r lib/test-requirements-with-tensorflow.txt --use-deprecated=legacy-resolver
+ pip install -r lib/test-requirements-with-tensorflow.txt
else
@echo ""
@echo "Your system does not support the official, pre-built tensorflow binaries."And then follow the instructions: tl;dr; # Build or rebuild the test env image. This normally takes a long time.
$ make build-test-env
# The -u flag tells Cypress to update snapshots.
$ ./scripts/run_e2e_tests.py -u e2e/specs/<name_of_test>.spec.js |
|
@vdonato @lukasmasuch : you're totally right. The Arrow thing was a red herring! It's just a warning 🤦 Fixed now. Thanks for the help debugging! |
|
Demo app looks great! Can you write QA plan and have this QAed whenever you're ready? I'm OOO until Friday, so you can skip my approval and just send it off to Kasim directly :) |
|
|
||
| // Formatted text | ||
| message DocString { | ||
|
|
||
| // The name of the object. | ||
| // The name the user gave to the variable holding this object. | ||
| string name = 1; |
There was a problem hiding this comment.
Whoops, my bad for not noticing this earlier, especially given that I have code ownership over these files specifically to enforce this 😅
We'll need to ensure that these proto changes are backwards compatible given that other stakeholders rely on them
We also want to be a bit more strict than normal proto backwards compatibility guidelines to also avoid changing field names to make things easier on folks when they're upgrading the OS lib version.
There was a problem hiding this comment.
Done. PTAL.
| // The name the user gave to the variable holding this object. | ||
| string name = 6; |
There was a problem hiding this comment.
nit: I think we could probably just keep the current name field vs adding this one, but not a big deal either way
There was a problem hiding this comment.
IMO the meaning changed enough that the right thing to do is to deprecate.
8d143c9 to
61625a6
Compare
* develop: StreamlitEndpoints.buildMediaURL (streamlit#6366) Set a default for RuntimeConfig.cache_storage_manager (streamlit#6361) FullScreenWrapper: add a type declaration for react context usage (streamlit#6364) Improve st.help (and st.write's usage of st.help!) (streamlit#5857) Fix regression with query_params (streamlit#6348) Have util.calc_md5 also take bytes (streamlit#6358) Improve deploy button (streamlit#6223) Return whether a secrets.toml file is successfully parsed (streamlit#6333) Add `.webp` to list of safe static file extensions (streamlit#6331) AppContext docstrings (streamlit#6353) fix: upgrade command-line-args from 5.0.2 to 5.2.1 (streamlit#6258) fix: upgrade flatbuffers from 1.11.0 to 1.12.0 (streamlit#6259) sendMessageToHost: no longer a global function (streamlit#6345) Tweak no-else-return config options (streamlit#6343) Allow users to set a secrets.toml file in their home directory (streamlit#6230) Add support for number and boolean types in categorical columns (streamlit#6248) Add support for period type in st.table and st.dataframe (streamlit#5429) Add ability to turn off anchors (streamlit#6158) Add sqlalchemy mypy types to test-requirements.txt (streamlit#6329)
📚 Context
As part of my hackathon project, I'm planning on submitting some fully-ready PRs (not prototypes!) that hopefully can be merged without a lot of work from the eng team. This is one of them! If it's too much work to merge, or the feature is deemed a bad idea, I'm happy to drop the whole thing.
What this PR does:
<foo object at 0x1234abcd>), calls st.help(the_object).This makes st.help, st.write, and magic great for quickly inspecting objects, classes, and what-have-yous! More info in the spec.
What kind of change does this PR introduce?
🧠 Description of Changes
Add bullet points summarizing your changes here
😬 Before
This is what you used to get for this code:
(etc)😎 After
(etc)🧪 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.