Port `dapp-runner` to `pydantic` #100

shadeofblue · 2023-03-09T08:23:00Z

remove the goth dependency from dapp-runner itself
remove the tailor-made BaseDescriptor
port DappDescriptor and its containing classes to pydantic
port ConfigDescriptor and its containing classes to pydantic

approxit · 2023-03-10T10:19:42Z

dapp_runner/descriptor/dapp.py

+        p = v.split(":")
+        return {"remote_port": p.pop(), "local_port": p.pop() if p else None}


Using p.pop() in this way requires way more attention, that it should... I would use more simple approach.

Suggested change

p = v.split(":")

return {"remote_port": p.pop(), "local_port": p.pop() if p else None}

p = v.split(":")

return {

"remote_port": p[0],

"local_port": p[1] if 2 <= len(p) else None,

}

Also, regex should enforce pattern of %d[:%d].

dapp_runner/descriptor/dapp.py

approxit · 2023-03-10T10:43:16Z

pyproject.toml

+_tests_integration_env = "python -m venv .envs/goth"
+_tests_integration_requirements = ".envs/goth/bin/pip install --extra-index-url https://test.pypi.org/simple/ goth==0.14.1 pytest pytest-asyncio pexpect"
+_tests_integration_assets = ".envs/goth/bin/python -m goth create-assets tests/goth_tests/assets"
+tests_integration_init = ["_tests_integration_env", "_tests_integration_requirements", "_tests_integration_assets"]


To be consistent with other tasks.

Suggested change

_tests_integration_env = "python -m venv .envs/goth"

_tests_integration_requirements = ".envs/goth/bin/pip install --extra-index-url https://test.pypi.org/simple/ goth==0.14.1 pytest pytest-asyncio pexpect"

_tests_integration_assets = ".envs/goth/bin/python -m goth create-assets tests/goth_tests/assets"

tests_integration_init = ["_tests_integration_env", "_tests_integration_requirements", "_tests_integration_assets"]

tests_integration_init = ["_tests_integration_env", "_tests_integration_requirements", "_tests_integration_assets"]

_tests_integration_env = "python -m venv .envs/goth"

_tests_integration_requirements = ".envs/goth/bin/pip install --extra-index-url https://test.pypi.org/simple/ goth==0.14.1 pytest pytest-asyncio pexpect"

_tests_integration_assets = ".envs/goth/bin/python -m goth create-assets tests/goth_tests/assets"

As diff looks terrible, the intention is to have parent task first, then children tasks.

approxit · 2023-03-10T10:50:40Z

dapp_runner/descriptor/dapp.py

+        extra = "forbid"
+
+    @validator("ports", pre=True, each_item=True)
+    def __ports_preprocess(cls, v):


Name is not consistent with __field_name__action.

approxit · 2023-03-10T10:54:58Z

dapp_runner/descriptor/dapp.py

 class SocketProxyDescriptor(ProxyDescriptor):
    """TCP socket proxy descriptor."""


-@dataclass
-class CommandDescriptor:
+def parse_command_descriptor(value: Union[str, List, Dict]):


Consider moving to classmethod of CommandDescriptor.

approxit · 2023-03-10T10:56:25Z

dapp_runner/descriptor/dapp.py

-@dataclass
-class NetworkDescriptor(BaseDescriptor["NetworkDescriptor"]):
+    @validator("init", pre=True)
+    def __init_commands(cls, v):


Name is not consistent with __field_name__action.

approxit · 2023-03-10T10:57:41Z

dapp_runner/descriptor/dapp.py

-class NetworkDescriptor(BaseDescriptor["NetworkDescriptor"]):
+    @validator("init", pre=True)
+    def __init_commands(cls, v):
+        if len(v) > 0 and isinstance(v[0], str):


Suggested change

if len(v) > 0 and isinstance(v[0], str):

if len(v) and isinstance(v[0], str):

approxit · 2023-03-10T11:14:12Z

dapp_runner/descriptor/dapp.py

+        self.__validate_nodes()
+        self.__implicit_proxy_init()
+        self.__implicit_vpn()
+        self.__implicit_manifest_support()
+        self._resolve_dependencies()


As this works, it could escape few behaviors as pydantic's author mentioned. This looks like not so complicated logic that could be implemented in pydantic's per-field validators. Also, back in my pydantic-like DTOs I've moved default values from parent models down to it's children. In result, I got simpler parent models. Consider similar approach for.

as mentioned on a call, these checks are not per-field and cannot be moved to the children

approxit · 2023-03-23T11:45:55Z

dapp_runner/descriptor/dapp.py

+        m = re.match("^((\\d+)\\:)?(\\d+)$", v)
+        if not m:
+            raise ValueError("Expected format: `remote_port` or `remote_port:local_port`.")
+
+        return {"remote_port": m.group(3), "local_port": m.group(2) if m.group(2) else None}


Looks like this regex can be improved.

Suggested change

m = re.match("^((\\d+)\\:)?(\\d+)$", v)

if not m:

raise ValueError("Expected format: `remote_port` or `remote_port:local_port`.")

return {"remote_port": m.group(3), "local_port": m.group(2) if m.group(2) else None}

m = re.match(r"^(?P<remote_port>\d+)(?::(?P<local_port>\d+))?$", v)

if not m:

raise ValueError("Expected format: `remote_port` or `remote_port:local_port`.")

return m.groupdict(}

shadeofblue added 4 commits March 6, 2023 09:50

remove goth's dependencies, add pydantic

dbc0fab

port the Config to pydantic

49b65cc

convert DappDescriptor to pydantic

9ee2844

remove the previous BaseDescriptor

adaa37e

shadeofblue requested review from approxit and lucekdudek March 9, 2023 08:23

shadeofblue self-assigned this Mar 9, 2023

shadeofblue force-pushed the blue/pydantic branch 2 times, most recently from 372bd38 to b7e1752 Compare March 10, 2023 06:23

re-enable goth tests

3e8009c

shadeofblue force-pushed the blue/pydantic branch from b7e1752 to 3e8009c Compare March 10, 2023 06:25

approxit reviewed Mar 10, 2023

View reviewed changes

shadeofblue linked an issue Mar 13, 2023 that may be closed by this pull request

port dapp-runner to pydantic #101

Closed

shadeofblue and others added 3 commits March 22, 2023 12:34

address review remarks

837fea8

Merge branch 'main' into blue/pydantic

dc2ebf2

fix the proxy entry descriptor preprocessor

b9c9188

shadeofblue marked this pull request as ready for review March 22, 2023 12:20

fix the goth run

6980a04

shadeofblue force-pushed the blue/pydantic branch from 048e410 to 6980a04 Compare March 23, 2023 07:11

make parse_command_descriptor a classmethod of CommandDescriptor

99e022c

approxit approved these changes Mar 23, 2023

View reviewed changes

better pattern match

ea2769c

shadeofblue force-pushed the blue/pydantic branch from cabe001 to ea2769c Compare March 23, 2023 15:00

try a different way to install poetry

3df6a63

shadeofblue merged commit e0181d2 into main Mar 23, 2023

shadeofblue deleted the blue/pydantic branch March 23, 2023 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port `dapp-runner` to `pydantic` #100

Port `dapp-runner` to `pydantic` #100

shadeofblue commented Mar 9, 2023

approxit Mar 10, 2023

approxit Mar 10, 2023

approxit Mar 10, 2023

approxit Mar 10, 2023

approxit Mar 10, 2023

approxit Mar 10, 2023

approxit Mar 10, 2023

shadeofblue Mar 23, 2023

approxit Mar 23, 2023

		p = v.split(":")
		return {"remote_port": p.pop(), "local_port": p.pop() if p else None}

	if len(v) > 0 and isinstance(v[0], str):
	if len(v) and isinstance(v[0], str):

Port dapp-runner to pydantic #100

Port dapp-runner to pydantic #100

Conversation

shadeofblue commented Mar 9, 2023

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

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

Choose a reason for hiding this comment

Port `dapp-runner` to `pydantic` #100

Port `dapp-runner` to `pydantic` #100