Skip to content
This repository has been archived by the owner on Apr 21, 2021. It is now read-only.

WIP: Node split #15

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

WIP: Node split #15

wants to merge 7 commits into from

Conversation

Lyla-Fischer
Copy link
Collaborator

@Lyla-Fischer Lyla-Fischer commented Jun 8, 2019

Addresses #12

I'm a little bit worried about "just" passing tests, without a more comprehensive test suite, considering that I was very much just fixing bugs as the tests revealed them, but this passes tests.

I put this on top of the string escaping, because I didn't want to deal with the inevitable conflicts.

@Lyla-Fischer Lyla-Fischer requested a review from ahopkins June 8, 2019 05:53
pydiggy/node.py Outdated Show resolved Hide resolved
@Lyla-Fischer
Copy link
Collaborator Author

Addressed some of the comments. I would still like to add more tests before merging this in, but one of the major contributors to untested code metrics is the existence of Node.save() and connections.py, which do more than just serialization/de-serialization, which (I think, based off of other discussions) is the focus of the current library.

I don't know if you want to improve the code coverage as part of this pull request or not, but I'm willing to wait for a merge to get some of that done, and make it more safe.

@@ -136,6 +136,10 @@ class Node(metaclass=NodeMeta):
_instances = dict()
_staged = dict()

# uid is not used as a class variable, it is part of the required
# schema of a Node, and is thus part of the superclass.
uid : int
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice documentation 💪

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants