Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update docker, add methods to load from dictionary, get tree traversa… #174

Merged
merged 6 commits into from
Nov 2, 2022
Merged

Update docker, add methods to load from dictionary, get tree traversa… #174

merged 6 commits into from
Nov 2, 2022

Conversation

itdependsnetworks
Copy link
Contributor

@itdependsnetworks itdependsnetworks commented Oct 30, 2022 •
edited
Loading

…l, and update docs

  • Update docker since it was failing with my build (needed gcc)
  • Updated poetry to accommodate other built issues with redislite
  • Added method to load from dictionary
  • Added method to show the traversal of tree, and docs to go with
  • Added method to get one or none

@itdependsnetworks
Copy link
Contributor Author

Note that mypy does not support recursive typing for now, hence the usage of Any.

glennmatthews
glennmatthews reviewed Oct 31, 2022
View reviewed changes
diffsync/__init__.py Outdated Show resolved Hide resolved
diffsync/__init__.py Outdated Show resolved Hide resolved
diffsync/__init__.py Outdated Show resolved Hide resolved
diffsync/__init__.py Outdated Show resolved Hide resolved
diffsync/utils.py Outdated Show resolved Hide resolved
diffsync/utils.py Outdated Show resolved Hide resolved
docs/source/getting_started/01-getting-started.md Outdated Show resolved Hide resolved
diffsync/__init__.py Outdated Show resolved Hide resolved
tests/unit/test_diffsync.py Outdated Show resolved Hide resolved
tests/unit/test_diffsync.py Show resolved Hide resolved
Kircheneer
Kircheneer reviewed Nov 1, 2022
View reviewed changes
diffsync/__init__.py Outdated Show resolved Hide resolved
Kircheneer
Kircheneer reviewed Nov 1, 2022
View reviewed changes
diffsync/utils.py Outdated Show resolved Hide resolved
Kircheneer
Kircheneer reviewed Nov 1, 2022
View reviewed changes
diffsync/utils.py Outdated Show resolved Hide resolved
Kircheneer
Kircheneer reviewed Nov 1, 2022
View reviewed changes
diffsync/utils.py Outdated Show resolved Hide resolved
Kircheneer
Kircheneer requested changes Nov 1, 2022
View reviewed changes
Copy link
Collaborator

@Kircheneer Kircheneer left a comment

Choose a reason for hiding this comment

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

Great stuff! I have a couple type hinting nitpicks but apart from that I like it.

@Kircheneer
Copy link
Collaborator

Note that mypy does not support recursive typing for now, hence the usage of Any.

I wasn't able to spot usage of typing.Any anywhere except in a function docstring (where the type was not equivalent to that in the signature). I say we just drop new type hints from docstrings (including the ones from this PR) and I make sure I remove all existing ones with #175 ?

@Kircheneer Kircheneer self-requested a review November 2, 2022 09:46
Kircheneer
Kircheneer approved these changes Nov 2, 2022
View reviewed changes
Copy link
Collaborator

@Kircheneer Kircheneer left a comment

Choose a reason for hiding this comment

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

LGTM

@Kircheneer Kircheneer merged commit 6f022dd into networktocode:develop Nov 2, 2022
@chadell chadell mentioned this pull request Nov 7, 2022
Merged
Kircheneer pushed a commit that referenced this pull request Nov 8, 2022
@itdependsnetworks @glennmatthews @Kircheneer
Update docker, add methods to load from dictionary, get tree traversa… (
fffae73 
#174)

* Update docker, add methods to load from dictionary, get tree traversal, and update docs

* Apply suggestions from code review

Co-authored-by: Glenn Matthews <[email protected]>

* Additional recommended updates for loop efficiency and using classmethod

* update docs

* mypy feedback

* lock poetry

Co-authored-by: Glenn Matthews <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glennmatthews glennmatthews glennmatthews left review comments

@Kircheneer Kircheneer Kircheneer approved these changes

@chadell chadell Awaiting requested review from chadell chadell is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@itdependsnetworks @Kircheneer @glennmatthews