Implements Tethys Reactpy App Scaffold #1081
Conversation
…ike original tethys gizmo added on_click, on_change, and on_mouse_over kwargs
There were a few bugs found when installing from a fresh test, namely: * The version of daphne installed by default didn't meet requirements * There was some experimental reactpy core development that I never backed out
There was a problem hiding this comment.
@shawncrawley This is amazing and I'm really excited to get it officially in Tethys. I have several questions (see inline).
In general I would prefer that pathlib.Path be used rather than os.path, but I won't hold this up for that if you don't have the bandwidth to change that now.
* Replaces all usage of os.path with pathlib.Path * reactpy_django detected and added to INSTALLED_APPS in settings.py * ReactPy App scaffold now has pyproject.toml instead of setup.py
In this one instance, Path.exists throws a "File too long" error on Unix machines, while os.path.exists dose not. I couldn't think of a good way around that for now.
|
@sdc50 I implemented your feedback. In full disclosure, I got a little carried away when swapping out os.path with pathlib.Path. I ended up removing os.path from the entire project, except for about two spots where pathlib.Path was actually more clunky or problematic. So this PR is definitely super gold-plated by scope creep at this point. That means that there are a lot of new modified files since your last review... I apologize in advance. I was already in too deep before I determined that I shouldn't have added all of that to this specific PR. |
There was a problem hiding this comment.
@shawncrawley just a couple of minor changes (I've added suggestions that you can just commit if you agree with them).
I'm very excited about the reactpy addition. The Path changes are all internal, but everything is converted back to a string in return values. I think this is the right call for now (to ensure we aren't breaking anything). However, in Tethys 5.0 I think we should switch and return Path objects wherever possible.
Co-authored-by: sdc50 <scott.d.christensen@erdc.dren.mil>
Co-authored-by: sdc50 <scott.d.christensen@erdc.dren.mil>
Adds tests for remaining tethys_component reactpy files Adds reactpy-django to standard install Fixes broken support for variable in url for pages Adds react-loading-overlay and react-map-gl to built-in ComponentLibrary support Fixes buggy use_workspace
|
@swainn @sdc50 I've got everything done and passing tests except oddly the "Tethys-CI / Conda Build (ubuntu-latest)" test run. Any thoughts there on what could be going on? I have reactpy-django now installed by default, which did throw a wrench in some of the testing. Maybe it's related here? But oddly only this one build... |
|
@gagelarsen Do you have any idea on the failing "Conda Build" GitHub workflow? |
The authors field cannot be present if name and email are blank, so they are now conditional upon those values being filled
|
I know the issue: it's definitely being caused by |
| app.root_url = "test-app" | ||
|
|
||
| app._registered_url_maps = [ | ||
| Namespace(name="exclude_page", title="Exclude Page", index=-1), |
There was a problem hiding this comment.
Why are these argparse.Namespaces? Seems like they should be simple dicts.
There was a problem hiding this comment.
I just wanted a mock-like instance of UrlMap... I should have just used a mock.MagicMock object, lol. I'll swap it for that.
There was a problem hiding this comment.
Turns out I must've chosen Namespace for a reason. Using mock.MagicMock causes the tests to fail because mocking the property name cannot be done on initialization (see mock docs). While I could have broken it out into two steps, it seemed like using Namespace shouldn't be a big deal. Let me know if you disagree.
There was a problem hiding this comment.
Try using the UrlMaps for the test instead of the Namespace.
tethys_cli/scaffold_templates/app_templates/reactpy/pyproject.toml_tmpl
Outdated
Show resolved
Hide resolved
Co-authored-by: Nathan Swain <swainn@users.noreply.github.com>
- pyproject.toml added to all scaffolds - reactpy_base.html refactored to extend app_base.html - minor cleanups and refactors
- Reverted last commit's swap of argparse.Namespace for mock.MagicMock since mock requires the "name" argument, which isn't allowed by MagicMock on initialization - Added reactpy to dependencies in environment.yml
|
It looks like the only thing that remains is getting |
Description
This merge request implements a Tethys ReactPy App scaffold. I will flesh out this description soon with all of the ins and outs of it. I just wanted to finally get this in as a draft pull request to get more eyes on it.
Changes Made to Code:
Related
Additional Notes
Quality Checks