-
Notifications
You must be signed in to change notification settings - Fork 69
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
Create two different api doc folders, one for Core and one for Express #1053
Conversation
f931ca8
to
e300809
Compare
e300809
to
189807e
Compare
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
if file == "app.py" or file.startswith("app-"): | ||
# Return relative app path | ||
app_paths.append(os.path.join(path, folder, file)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will now add any variation of app.py
, app-core.py
, app-express.py
, app-other-stuff-core.py
, etc. to the example tests.
This lets us test our Express and Core example variations, but expands the total apps being tested up to ~160 as of right now, probably over 225+ once all examples are translated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the best testing strategy for express? I feel like testing both versions of each app is a bit overkill since it's basically running the same app twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we want to test all the apps that we're using as examples
_objects_core.json | ||
_objects_express.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@schloerke is it useful to keep these around? Or should we remove them as a part of python _combine_objects_json.py
?
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
shiny/_docstring.py
Outdated
def no_example_express(decorator: Callable[..., F]) -> F | Callable[..., F]: | ||
""" | ||
Prevent ``@add_example()`` from throwing an error about missing Express examples. | ||
""" | ||
|
||
@wraps(decorator) | ||
def wrapper_decorator(*args: Any, **kwargs: Any) -> F: | ||
try: | ||
# Apply the potentially problematic decorator | ||
return decorator(*args, **kwargs) | ||
except ExpressExampleNotFoundException: | ||
# If an error occurs, return the original function | ||
if args and callable(args[0]): | ||
return args[0] | ||
raise | ||
|
||
return wrapper_decorator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, instead of adding another decorator, it seems better if @no_example
took a mode
? Like @no_example(mode = "express")
?
That said, given the timeline on this, I'd be fine with how it is now, especially if not gonna have a need for it relatively soon. One thing I ask is that, if we plan on doing something like what I propose above, let's add a TODO/NOTE about it in check_if_missing_expected_example
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion! I updated @no_example()
to be used like this:
@add_example()
@no_example("express")
def a_core_thing_only():
pass
A couple caveats/notes:
- You now need to call
@no_example()
where before we used@no_example
@no_example()
needs to come before any@add_example()
decorators@no_example()
alone ignores both, or"express"
or"core"
to ignore individually. (You can stack these.)@no_example()
needs to be called without the named mode arg. (We can address this later if its an issue; in short its because we're looking at literal strings in quartodoc and they need to beno_example()
,no_example("express")
orno_example("core")
.)
* @no_example() ignores both * @no_example('core') ignores only core * @no_example('express') ignores only express
if x == "no_example()": | ||
return True | ||
|
||
return x == f'no_example("{os.environ.get("SHINY_MODE", "core")}")' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to be worried about people potentially using single-quote?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. Our linters won't let them and I think the string we see in quartodoc is standardized anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can probably be improved, but I'm okay with it for now. It's not blocking and if it becomes problematic, issues would surface in CI on PRs first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once final suggestions are addressed
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
* main: (33 commits) test: Test apps locally before deploying via pytest fixtures. (#1055) docs: Add ExtendedTask to API index (#1088) Fix `render.download` in Shiny Express, take 2 (#1085) Bump version to 0.7.0.9000 Create two different api doc folders, one for Core and one for Express (#1053) chore: Pin black to version 23 (#1077) chore: Remove github link to shinylive (#1069) Bump version to 0.7.0 Raise when `express.[input,output,session]` are used outside of Express app (#1067) Update dashboard template (#1056) chore: Remove many broken quartodoc links (#1061) Update {bslib} (#1062) Update docstrings for `expressify`, `hold`, and `render.express` (#1066) Add `fill` to `__all__` in `ui` and `express.ui` (#1064) Update shiny.js (#1059) docs(examples): Use refactored shinylive syntax (#1048) Update `shiny.js` (#1052) Add `express.ui.panel_title` (#1039) Don't run `effect`s created in a `MockSession` (#1049) Delete shiny/api-examples/Calc directory (#1044) ...
Many Express functions are currently missing examples. This will cause the build to fail unless you make the following change: in docs/_renderer.py, change the
check_if_missing_expected_example
function to return immediately.