revert #841 #919

rmorshea · 2023-02-10T03:00:32Z

closes: #920

and reverts changes from #841 as per discussion in #916

but preserve ability to declare attributes with snake_case

Checklist

Please update this checklist as you complete each item:

Tests have been included for all bug fixes or added functionality.
The changelog.rst has been updated with any significant changes.
GitHub Issues which may be closed by this Pull Request have been linked.

src/idom/core/vdom.py

scripts/fix_vdom_constructor_usage.py

src/idom/core/vdom.py

src/idom/html.py

src/idom/utils.py

tests/test_core/test_serve.py

.github/workflows/test.yml

.pre-commit-config.yaml

rmorshea · 2023-02-13T01:27:17Z

@Archmonger I think we can go forward with the rename after this.

Archmonger · 2023-02-13T01:20:29Z

docs/examples.py

@@ -122,7 +122,7 @@ def Wrapper():
    def PrintView():
        text, set_text = idom.hooks.use_state(print_buffer.getvalue())
        print_buffer.set_callback(set_text)
-        return idom.html.pre(text, class_name="printout") if text else idom.html.div()
+        return idom.html.pre({"class": "printout"}, text) if text else idom.html.div()


"class" needs a revert to "class_name". Also confirm throughout the docs we aren't missing any of those situations.

Suggested change

return idom.html.pre({"class": "printout"}, text) if text else idom.html.div()

return idom.html.pre({"class_name": "printout"}, text) if text else idom.html.div()

This appears to still be unresolved?

docs/source/about/changelog.rst

docs/source/guides/adding-interactivity/components-with-state/_examples/isolated_state/main.py

src/idom/_console/rewrite_key_declarations.py

src/idom/core/hooks.py

src/idom/html.py

src/idom/utils.py

tests/test_core/test_component.py

docs/source/reference/_examples/simple_dashboard.py

- but preserve ability to declare attributes with snake_case - move 'key' to be an attribute rather than keyword argument in constructor + make auto converter for this change based on the one used for the "new" `*args` and `**kwargs` syntax we are reverting.

rmorshea · 2023-02-21T08:58:18Z

I'd like to get this out, so I'm going to merge and do a pre-release

Archmonger · 2023-02-21T10:37:49Z

I'll do a post-merge review tomorrow. If there's any issues we can handle it in a separate release.

src/idom/_console/rewrite_camel_case_props.py

src/idom/_console/rewrite_keys.py

src/idom/_console/rewrite_camel_case_props.py

Archmonger · 2023-02-22T00:50:20Z

docs/examples.py

@@ -122,7 +122,7 @@ def Wrapper():
    def PrintView():
        text, set_text = idom.hooks.use_state(print_buffer.getvalue())
        print_buffer.set_callback(set_text)
-        return idom.html.pre(text, class_name="printout") if text else idom.html.div()
+        return idom.html.pre({"class": "printout"}, text) if text else idom.html.div()


This appears to still be unresolved?

Archmonger · 2023-02-22T00:50:49Z

docs/source/_exts/custom_autosectionlabel.py

@@ -30,6 +30,7 @@ def get_node_depth(node: Node) -> int:

 def register_sections_as_label(app: Sphinx, document: Node) -> None:
    docname = app.env.docname
+    print(docname)


Unneeded print?

Archmonger · 2023-02-22T00:50:58Z

docs/source/_exts/custom_autosectionlabel.py

@@ -66,7 +67,7 @@ def register_sections_as_label(app: Sphinx, document: Node) -> None:
        domain.labels[name] = docname, labelid, sectname


-def setup(app: Sphinx) -> dict[str, Any]:
+def setup(app: Sphinx) -> Dict[str, Any]:


No need to use typing.Dict here.

Archmonger · 2023-02-22T00:52:00Z

docs/source/guides/adding-interactivity/dangers-of-mutability/_examples/moving_dot_broken.py

+            {
+                "style": {
+                    "position": "absolute",
+                    "backgroundColor": "red",


Should we convert styles to snake_case?

Archmonger · 2023-02-22T00:52:45Z

docs/source/guides/adding-interactivity/multiple-state-updates/_examples/set_color_3_times.py

@@ -15,9 +15,11 @@ def handle_reset(event):

    return html.div(
        html.button(
-            "Set Color", on_click=handle_click, style={"background_color": color}
+            {"on_click": handle_click, "style": {"backgroundColor": color}}, "Set Color"


Tons of backgroundColor references in the docs. We might want them to be background_color.

Archmonger · 2023-02-22T02:58:21Z

tests/test_core/test_serve.py

@@ -115,8 +115,8 @@ async def handle_event():
            second_event_did_execute.set()

        return idom.html.div(
-            idom.html.button(on_click=block_forever),
-            idom.html.button(on_click=handle_event),
+            idom.html.button({"onClick": block_forever}),


onClick -> on_click
There's a few instances of unconverted values in the tests. This might indicate the conversion script isn't working in some edge cases.

Archmonger · 2023-02-22T03:00:14Z

docs/source/guides/adding-interactivity/state-as-a-snapshot/_examples/print_chat_message.py

        html.label(
+            {},


There's a handful of these empty dicts in this PR.

I recommend doing a search through this diff page for {}

Archmonger · 2023-02-23T00:51:28Z

Been testing this out in Conreq. I have a couple of comments

Using custom_vdom_constructor on _ and script elements causes pylint errors
- We should probably handle this through overloads and helper functions instead.
On some of my idom.html elements, I was constructing the vdom dict using function calls (ex. generate_props(val_1, val_2)). The rewrite-keys script then would add another vdom dict above it, ultimately breaking things.
- Should we update the script to run a type-check on the first arg of an idom.html call (and then combine them with the pipe | operator if needed)?
- The following error probably needs more verbosity in order to better catch things like this:

  File "C:\Users\Markg\Documents\Repositories\Conreq\.venv\lib\site-packages\idom\core\layout.py", line 202, in _render_model
    new_state.model.current = {"tagName": raw_model["tagName"]}
KeyError: 'tagName'

rmorshea · 2023-02-23T02:49:14Z

Unfortunately I don't think there's a good great way handle that case you're experiencing. That first posarg could also be a child rather than a props dict. I think we can just produce a more helpful error.

Archmonger · 2023-02-23T02:50:53Z

Yeah no problem. To be honest it probably doesn't need the extra robustness, since as far as I can tell I'm the only one "stress testing" IDOM.

The error improvement would be a good addition though

Archmonger reviewed Feb 10, 2023

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

Archmonger reviewed Feb 10, 2023

View reviewed changes

.pre-commit-config.yaml Show resolved Hide resolved

rmorshea force-pushed the revert-764a5a8c branch 8 times, most recently from 42eb96d to 1e64cf7 Compare February 13, 2023 00:29

rmorshea marked this pull request as ready for review February 13, 2023 00:45

rmorshea requested a review from Archmonger February 13, 2023 01:18

Archmonger reviewed Feb 13, 2023

View reviewed changes

Archmonger reviewed Feb 15, 2023

View reviewed changes

docs/source/reference/_examples/simple_dashboard.py Show resolved Hide resolved

rmorshea added 14 commits February 20, 2023 23:57

revert 764a5a8

78a29b8

- but preserve ability to declare attributes with snake_case - move 'key' to be an attribute rather than keyword argument in constructor + make auto converter for this change based on the one used for the "new" `*args` and `**kwargs` syntax we are reverting.

add key rewrite script

b253e26

changelog

05da013

fix types

6eab33f

apply rewrites

a6a71bc

add custom vdom constructor decorator

e421c3a

change to snake

ec1eb38

fix err msg

0e87c0a

cast keys to strings

2f95d58

make pub

78623a0

fix dashed html attrs

3c03550

rename to rewrite-keys

06628f4

fix types

d53154d

allow ints in vdom spec for keys

8d8e0b6

rmorshea added 2 commits February 20, 2023 23:57

fix types

7ee7b98

camelCase to snake_case + rewrite util

f1bf70c

rmorshea force-pushed the revert-764a5a8c branch from 51aa05c to f1bf70c Compare February 21, 2023 07:57

rmorshea added 3 commits February 21, 2023 00:04

fix types

564286a

get cov

36b5d82

fix style

76234d6

rmorshea requested a review from Archmonger February 21, 2023 08:18

rmorshea merged commit f0ff025 into main Feb 21, 2023

rmorshea deleted the revert-764a5a8c branch February 21, 2023 08:58

Archmonger reviewed Feb 21, 2023

View reviewed changes

src/idom/_console/rewrite_camel_case_props.py Show resolved Hide resolved

Archmonger reviewed Feb 22, 2023

View reviewed changes

This was referenced Feb 23, 2023

Rewrite style props #936

Merged

Fix pylint issues #937

Merged

Archmonger mentioned this pull request Nov 4, 2023

Rethink snake_case to camelCase prop conversion #1160

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

revert #841 #919

revert #841 #919

rmorshea commented Feb 10, 2023 •

edited

Loading

rmorshea commented Feb 13, 2023

Archmonger Feb 13, 2023

rmorshea Feb 13, 2023

Archmonger Feb 22, 2023

rmorshea commented Feb 21, 2023

Archmonger commented Feb 21, 2023 •

edited

Loading

Archmonger Feb 22, 2023

Archmonger Feb 22, 2023 •

edited

Loading

Archmonger Feb 22, 2023

Archmonger Feb 22, 2023

Archmonger Feb 22, 2023

Archmonger Feb 22, 2023 •

edited

Loading

Archmonger Feb 22, 2023

Archmonger commented Feb 23, 2023 •

edited

Loading

rmorshea commented Feb 23, 2023

Archmonger commented Feb 23, 2023 •

edited

Loading

	return idom.html.pre({"class": "printout"}, text) if text else idom.html.div()
	return idom.html.pre({"class_name": "printout"}, text) if text else idom.html.div()

revert #841 #919

revert #841 #919

Conversation

rmorshea commented Feb 10, 2023 • edited Loading

Checklist

rmorshea commented Feb 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmorshea commented Feb 21, 2023

Archmonger commented Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

Archmonger Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Archmonger Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Archmonger commented Feb 23, 2023 • edited Loading

rmorshea commented Feb 23, 2023

Archmonger commented Feb 23, 2023 • edited Loading

rmorshea commented Feb 10, 2023 •

edited

Loading

Archmonger commented Feb 21, 2023 •

edited

Loading

Archmonger Feb 22, 2023 •

edited

Loading

Archmonger Feb 22, 2023 •

edited

Loading

Archmonger commented Feb 23, 2023 •

edited

Loading

Archmonger commented Feb 23, 2023 •

edited

Loading