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

Feature big bank example #19

Merged
merged 61 commits into from
Aug 13, 2020
Merged

Feature big bank example #19

merged 61 commits into from
Aug 13, 2020

Conversation

Copy link
Owner

@Midnighter Midnighter left a comment

Choose a reason for hiding this comment

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

You're killing me with these huge PRs 😅 but I'm very grateful for all the work. I left a bunch of comments. Most are simple changes.

We should come to a design decision for the hydrate and hydrate_arguments methods. Probably best to discuss that in an issue.

src/structurizr/model/container.py Outdated Show resolved Hide resolved
src/structurizr/api/structurizr_client.py Show resolved Hide resolved
src/structurizr/model/component.py Outdated Show resolved Hide resolved
src/structurizr/model/element.py Show resolved Hide resolved
src/structurizr/model/element.py Outdated Show resolved Hide resolved
src/structurizr/view/view_set.py Outdated Show resolved Hide resolved
src/structurizr/view/view_set.py Show resolved Hide resolved
src/structurizr/view/view_set.py Show resolved Hide resolved
src/structurizr/workspace.py Show resolved Hide resolved
src/structurizr/workspace.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Aug 10, 2020

Codecov Report

Merging #19 into devel will increase coverage by 1.04%.
The diff coverage is 63.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##            devel      #19      +/-   ##
==========================================
+ Coverage   79.39%   80.43%   +1.04%     
==========================================
  Files          61       63       +2     
  Lines        1412     1661     +249     
  Branches       82      129      +47     
==========================================
+ Hits         1121     1336     +215     
- Misses        265      290      +25     
- Partials       26       35       +9     
Impacted Files Coverage Δ
src/structurizr/model/static_structure_element.py 78.57% <0.00%> (ø)
src/structurizr/view/system_landscape_view.py 89.47% <ø> (+21.05%) ⬆️
src/structurizr/api/structurizr_client.py 48.06% <16.66%> (-1.14%) ⬇️
src/structurizr/view/view.py 61.36% <34.09%> (-27.32%) ⬇️
src/structurizr/model/model.py 76.51% <46.66%> (+4.51%) ⬆️
src/structurizr/view/element_style.py 97.82% <50.00%> (+0.09%) ⬆️
src/structurizr/view/element_view.py 88.00% <50.00%> (-7.00%) ⬇️
src/structurizr/view/system_context_view.py 90.00% <50.00%> (-4.45%) ⬇️
src/structurizr/view/view_set.py 59.34% <55.95%> (-2.92%) ⬇️
src/structurizr/view/component_view.py 63.41% <63.41%> (ø)
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f991e6f...156d398. Read the comment docs.

@@ -820,3 +820,6 @@ flycheck_*.el

# Until we have sensible defaults, ignore VS code
/.vscode/

# Ignore default generated workspace DSL from get_workspace
structurizr-*.json.gz
Copy link
Owner

Choose a reason for hiding this comment

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

This should probably go into your .git/info/exclude.

Copy link
Owner

@Midnighter Midnighter left a comment

Choose a reason for hiding this comment

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

It's quite hard to see which of the comments you have all implemented but I trust that you did the right thing 😃 One comment about the test case otherwise looks good!

examples/big_bank.py Outdated Show resolved Hide resolved
@@ -73,4 +70,5 @@ def test_serialize_workspace(example, filename, monkeypatch):
# TODO (midnighter): Use `from_orm` like `.construct` bypassing validation. (
# Requires a pull request on pydantic.)
expected = WorkspaceIO.from_orm(Workspace.load(path))
assert WorkspaceIO.from_orm(example.main()) == expected
actual = WorkspaceIO.from_orm(example.main())
assert json.loads(actual.json()) == json.loads(expected.json())
Copy link
Owner

Choose a reason for hiding this comment

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

Why not use .dict() here? Or do you definitely want to load the JSON? In that case you could change it to:

Suggested change
assert json.loads(actual.json()) == json.loads(expected.json())
assert json.loads(actual.json()) == json.load(path)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's actually what I meant to do since our default values are not populated exactly like in the original examples. Mostly because the empty objects are populated in the C# SDK, but not default values, a behavior that I didn't find an easy way to reproduce exactly.

So, just to understand, the test that I'm doing is not perfect since it manipulates the expected JSON... But for now, that's what we have. I think that we should dive deeper into unit tests to make sure the functionality matches.

Copy link
Owner

Choose a reason for hiding this comment

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

My point is that:

json.loads(actual.json())

is the same as

actual.dict()

@Midnighter Midnighter force-pushed the feature-big-bank-example branch from a004553 to 156d398 Compare August 13, 2020 18:08
Copy link
Owner

@Midnighter Midnighter left a comment

Choose a reason for hiding this comment

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

Applied most comments now. It got a bit messy because the GitHub UI is hiding the majority of the comments. Thanks a lot for this chunk of work!

@Midnighter Midnighter merged commit 9396d69 into devel Aug 13, 2020
@Midnighter Midnighter deleted the feature-big-bank-example branch August 13, 2020 18:10
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.

3 participants